Re: [PATCH v2] io_uring: propagate array_index_nospec opcode into req->opcode
Caleb Sander Mateos <[email protected]>
| Newsgroups | org.kernel.vger.io-uring,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <CADUfDZpB__CEbcjwtOOuHCwVcnD1yEAx8F+8EJ3UqXY4YJCj0A@mail.gmail.com> |
On Mon, May 18, 2026 at 7:49 AM Jens Axboe <[email protected]> wrote: > > On 5/18/26 8:42 AM, Caleb Sander Mateos wrote: > > On Sun, May 17, 2026 at 2:30?PM Michael Bommarito > > <[email protected]> wrote: > >> > >> Commit 1e988c3fe126 ("io_uring: prevent opcode speculation") added > >> array_index_nospec() to io_init_req(), but applied it only to a local > >> opcode variable. req->opcode is initialized from sqe->opcode before the > >> bounds check and remains the raw value. > >> > >> Keep req->opcode as the canonical opcode in io_init_req(): reject > >> out-of-range values architecturally, then write the array_index_nospec() > >> result back to req->opcode before any table lookup. This keeps downstream > >> users of req->opcode from observing the raw user byte on a mispredicted > >> path. > >> > >> No functional change: array_index_nospec() is a no-op for opcodes in > >> [0, IORING_OP_LAST), and out-of-range opcodes are still rejected at the > >> bounds check above the assignment. Boot-tested under UML (x86_64 > >> defconfig) by building stock and patched kernels and running a 54-test > >> subset of liburing against each; pass/fail results were identical. > >> > >> Fixes: 1e988c3fe126 ("io_uring: prevent opcode speculation") > >> > >> Assisted-by: Claude:claude-opus-4-7 > >> Signed-off-by: Michael Bommarito <[email protected]> > >> --- > >> v2: > >> - Fold the clamped value into req->opcode and use req->opcode for > >> the io_issue_defs[] lookup, rather than keeping a second local > >> opcode variable. Suggested by Jens. > >> - Keep the hardening-only framing; no functional behavior change. > >> > >> io_uring/io_uring.c | 9 ++++----- > >> 1 file changed, 4 insertions(+), 5 deletions(-) > >> > >> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c > >> index 4ed998d60c09c..84e16c3ad3f47 100644 > >> --- a/io_uring/io_uring.c > >> +++ b/io_uring/io_uring.c > >> @@ -1721,10 +1721,9 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, > >> const struct io_issue_def *def; > >> unsigned int sqe_flags; > >> int personality; > >> - u8 opcode; > >> > >> req->ctx = ctx; > >> - req->opcode = opcode = READ_ONCE(sqe->opcode); > >> + req->opcode = READ_ONCE(sqe->opcode); > > > > The local variable should improve performance, I'm not sure removing > > it is a good idea. Due to the intervening stores, the compiler can't > > tell that req->opcode is unchanged between this assignment and the > > later loads, so it will have to reload it from memory. Can you just > > assign to the local variable opcode here and wait to assign to > > req->opcode until after updating opcode with array_index_nospec()? > > It generated the same code on my end, using gcc and arm64. If that's not > the case for you, yeah then retaining the local variable would be fine > too, like v1 did. Oh, I missed that the only stores in between are to other fields of *req. Yeah, the compiler should be able to tell that those don't alias req->opcode. Removing the local variable sounds good. Thanks, Caleb