[PATCH] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read

[PATCH] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read

[Jann Horn]

cmd->sqe seems to point to shared memory here; so values should only be
read from it with READ_ONCE(). To ensure that the compiler won't generate
code that assumes the value in memory will stay constant, add a
READ_ONCE().
The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already
do this correctly.

Signed-off-by: Jann Horn <jannh@google.com>
---
 io_uring/uring_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index fc94c465a9850d4ed9df0cd26fcd6523657a2854..f4397bd66283d5939b60e7fa0a12bd7426322b9f 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -350,7 +350,7 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags)
 	if (!prot || !prot->ioctl)
 		return -EOPNOTSUPP;
 
-	switch (cmd->sqe->cmd_op) {
+	switch (READ_ONCE(cmd->sqe->cmd_op)) {
 	case SOCKET_URING_OP_SIOCINQ:
 		ret = prot->ioctl(sk, SIOCINQ, &arg);
 		if (ret)

---
base-commit: 95ec54a420b8f445e04a7ca0ea8deb72c51fe1d3
change-id: 20250121-uring-sockcmd-fix-75b73e5b9750

-- 
Jann Horn <jannh@google.com>

Re: [PATCH] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read

[Jens Axboe]

On Tue, 21 Jan 2025 17:09:59 +0100, Jann Horn wrote:
> cmd->sqe seems to point to shared memory here; so values should only be
> read from it with READ_ONCE(). To ensure that the compiler won't generate
> code that assumes the value in memory will stay constant, add a
> READ_ONCE().
> The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already
> do this correctly.
> 
> [...]

Applied, thanks!

[1/1] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read
      commit: 0963dba3dc006b454c54fd019bbbdb931e7a7c70

Best regards,
-- 
Jens Axboe

Re: [PATCH] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read

[Jens Axboe]

On 1/22/25 12:38 PM, Jens Axboe wrote:
> 
> On Tue, 21 Jan 2025 17:09:59 +0100, Jann Horn wrote:
>> cmd->sqe seems to point to shared memory here; so values should only be
>> read from it with READ_ONCE(). To ensure that the compiler won't generate
>> code that assumes the value in memory will stay constant, add a
>> READ_ONCE().
>> The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already
>> do this correctly.
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read
>       commit: 0963dba3dc006b454c54fd019bbbdb931e7a7c70

I took a closer look and this isn't necessary. Either ->sqe is a full
copy at this point. Should probably be renamed as such... If we want to
make this clearer, then we should do:

-       switch (cmd->sqe->cmd_op) {
+       switch (cmd->cmd_op) {

instead.

I'll drop this one.

-- 
Jens Axboe

Re: [PATCH] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read

[Jann Horn]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On Thu, Jan 23, 2025 at 1:18 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/22/25 12:38 PM, Jens Axboe wrote:
> >
> > On Tue, 21 Jan 2025 17:09:59 +0100, Jann Horn wrote:
> >> cmd->sqe seems to point to shared memory here; so values should only be
> >> read from it with READ_ONCE(). To ensure that the compiler won't generate
> >> code that assumes the value in memory will stay constant, add a
> >> READ_ONCE().
> >> The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already
> >> do this correctly.
> >>
> >> [...]
> >
> > Applied, thanks!
> >
> > [1/1] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read
> >       commit: 0963dba3dc006b454c54fd019bbbdb931e7a7c70
>
> I took a closer look and this isn't necessary. Either ->sqe is a full
> copy at this point. Should probably be renamed as such... If we want to
> make this clearer, then we should do:

Are you sure? On mainline (at commit 21266b8df522), I applied the
attached diff that basically adds some printf debugging and adds this
in io_uring_cmd_sock():

pr_warn("%s: [first read] cmd->sqe->cmd_op = 0x%x\n", __func__,
READ_ONCE(cmd->sqe->cmd_op));
mdelay(2000);
pr_warn("%s: [second read] cmd->sqe->cmd_op = 0x%x\n", __func__,
READ_ONCE(cmd->sqe->cmd_op));

Then I ran the attached testcase, which submits a SQE and then
modifies the ->cmd_op of the SQE while it is being submitted.

Resulting dmesg output, showing that cmd->sqe->cmd_op changes when
userspace modifies the SQE:

[  180.415944][ T1110] io_submit_sqes: SQE = ffff888010bcc000
[  180.418731][ T1110] io_submit_sqe: SQE = ffff888010bcc000
[  180.421191][ T1110] io_queue_sqe
[  180.422160][ T1110] io_issue_sqe
[  180.423101][ T1110] io_uring_cmd: SQE = ffff888010bcc000
[  180.424570][ T1110] io_uring_cmd_sock: cmd->sqe = ffff888010bcc000
[  180.426272][ T1110] io_uring_cmd_sock: [first read] cmd->sqe->cmd_op = 0x1234
[  182.429036][ T1110] io_uring_cmd_sock: [second read]
cmd->sqe->cmd_op = 0x5678

[-- Attachment #2: uring-cmd-test.c --]
[-- Type: text/x-csrc, Size: 2033 bytes --]

#define _GNU_SOURCE
#include <pthread.h>
#include <err.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <poll.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sys/prctl.h>
#include <sys/eventfd.h>
#include <sys/syscall.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <linux/io_uring.h>

#define SYSCHK(x) ({          \
  typeof(x) __res = (x);      \
  if (__res == (typeof(x))-1) \
    err(1, "SYSCHK(" #x ")"); \
  __res;                      \
})

#define NUM_SQ_PAGES 4

static int uring_fd;
static struct io_uring_sqe *sq_data;

static void *thread_fn(void *dummy) {
  sleep(1);
  sq_data->cmd_op = 0x5678;
  return NULL;
}

int main(void) {
  printf("main pid: %d\n", getpid());

  // sq
  sq_data = SYSCHK(mmap(NULL, NUM_SQ_PAGES*0x1000, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_ANONYMOUS, -1, 0));
  // cq
  void *cq_data = SYSCHK(mmap(NULL, NUM_SQ_PAGES*0x1000, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_ANONYMOUS, -1, 0));
  *(volatile unsigned int *)(cq_data+4) = 64 * NUM_SQ_PAGES;

  // initialize uring
  struct io_uring_params params = {
    .flags = IORING_SETUP_DEFER_TASKRUN|IORING_SETUP_SINGLE_ISSUER|IORING_SETUP_NO_MMAP|IORING_SETUP_NO_SQARRAY,
    .sq_off = { .user_addr = (unsigned long)sq_data },
    .cq_off = { .user_addr = (unsigned long)cq_data }
  };
  uring_fd = SYSCHK(syscall(__NR_io_uring_setup, /*entries=*/10, &params));

  int sockfd = SYSCHK(socket(AF_INET, SOCK_STREAM, 0));

  sq_data[0] = (struct io_uring_sqe) {
    .opcode = IORING_OP_URING_CMD,
    .flags = 0,
    .ioprio = 0,
    .fd = sockfd,
    .cmd_op = 0x1234,
    .user_data = 123
  };

  pthread_t thread;
  if (pthread_create(&thread, NULL, thread_fn, NULL))
    errx(1, "pthread_create");

  int submitted = SYSCHK(syscall(__NR_io_uring_enter, uring_fd,
                                 /*to_submit=*/1, /*min_complete=*/1,
                                 /*flags=*/IORING_ENTER_GETEVENTS, /*sig=*/NULL, /*sigsz=*/0));
  printf("submitted %d\n", submitted);
  return 0;
}

[-- Attachment #3: uring-cmd-test.diff --]
[-- Type: text/x-patch, Size: 2704 bytes --]

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 7bfbc7c22367..0ae830faf0d9 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1723,6 +1723,8 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
 	const struct cred *creds = NULL;
 	int ret;
 
+	pr_warn("%s\n", __func__);
+
 	if (unlikely(!io_assign_file(req, def, issue_flags)))
 		return -EBADF;
 
@@ -1942,6 +1944,8 @@ static inline void io_queue_sqe(struct io_kiocb *req)
 {
 	int ret;
 
+	pr_warn("%s\n", __func__);
+
 	ret = io_issue_sqe(req, IO_URING_F_NONBLOCK|IO_URING_F_COMPLETE_DEFER);
 
 	/*
@@ -2159,6 +2163,8 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 	struct io_submit_link *link = &ctx->submit_state.link;
 	int ret;
 
+	pr_warn("%s: SQE = %px\n", __func__, sqe);
+
 	ret = io_init_req(ctx, req, sqe);
 	if (unlikely(ret))
 		return io_submit_fail_init(sqe, req, ret);
@@ -2187,6 +2193,7 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 
 	} else if (unlikely(req->flags & (IO_REQ_LINK_FLAGS |
 					  REQ_F_FORCE_ASYNC | REQ_F_FAIL))) {
+		pr_warn("%s: not queuing SQE %px, flags=0x%llx\n", __func__, sqe, req->flags);
 		if (req->flags & IO_REQ_LINK_FLAGS) {
 			link->head = req;
 			link->last = req;
@@ -2309,6 +2316,7 @@ int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr)
 			io_req_add_to_cache(req, ctx);
 			break;
 		}
+		pr_warn("%s: SQE = %px\n", __func__, sqe);
 
 		/*
 		 * Continue submitting even for sqe failure if the
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index fc94c465a985..5d88c72d6a89 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -6,6 +6,7 @@
 #include <linux/io_uring/net.h>
 #include <linux/security.h>
 #include <linux/nospec.h>
+#include <linux/delay.h>
 #include <net/sock.h>
 
 #include <uapi/linux/io_uring.h>
@@ -235,6 +236,8 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
 	struct file *file = req->file;
 	int ret;
 
+	pr_warn("%s: SQE = %px\n", __func__, ioucmd->sqe);
+
 	if (!file->f_op->uring_cmd)
 		return -EOPNOTSUPP;
 
@@ -347,9 +350,15 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags)
 	struct proto *prot = READ_ONCE(sk->sk_prot);
 	int ret, arg = 0;
 
+	pr_warn("%s: cmd->sqe = %px\n", __func__, cmd->sqe);
+
 	if (!prot || !prot->ioctl)
 		return -EOPNOTSUPP;
 
+	pr_warn("%s: [first read] cmd->sqe->cmd_op = 0x%x\n", __func__, READ_ONCE(cmd->sqe->cmd_op));
+	mdelay(2000);
+	pr_warn("%s: [second read] cmd->sqe->cmd_op = 0x%x\n", __func__, READ_ONCE(cmd->sqe->cmd_op));
+
 	switch (cmd->sqe->cmd_op) {
 	case SOCKET_URING_OP_SIOCINQ:
 		ret = prot->ioctl(sk, SIOCINQ, &arg);

Re: [PATCH] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read

[Jens Axboe]

On 1/23/25 7:44 AM, Jann Horn wrote:
> On Thu, Jan 23, 2025 at 1:18?AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/22/25 12:38 PM, Jens Axboe wrote:
>>>
>>> On Tue, 21 Jan 2025 17:09:59 +0100, Jann Horn wrote:
>>>> cmd->sqe seems to point to shared memory here; so values should only be
>>>> read from it with READ_ONCE(). To ensure that the compiler won't generate
>>>> code that assumes the value in memory will stay constant, add a
>>>> READ_ONCE().
>>>> The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already
>>>> do this correctly.
>>>>
>>>> [...]
>>>
>>> Applied, thanks!
>>>
>>> [1/1] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read
>>>       commit: 0963dba3dc006b454c54fd019bbbdb931e7a7c70
>>
>> I took a closer look and this isn't necessary. Either ->sqe is a full
>> copy at this point. Should probably be renamed as such... If we want to
>> make this clearer, then we should do:
> 
> Are you sure? On mainline (at commit 21266b8df522), I applied the
> attached diff that basically adds some printf debugging and adds this
> in io_uring_cmd_sock():

Yeah you are right, braino on my part. If we don't go async, it's not
copied. The changed fix is still better, but I'll reword the commit
message to be more accurate.

-- 
Jens Axboe