Re: [PATCH] scsi: bsg: copy uring_cmd payload to prevent double-fetch from shared SQE

On 5/27/26 10:45 AM, Jens Axboe wrote:
> On 5/27/26 10:27 AM, Caleb Sander Mateos wrote:
>> On Wed, May 27, 2026 at 9:19?AM Rahul Chandelkar <[email protected]> wrote:
>>>
>>> On Wed, May 27, 2026 at 10:06:44AM -0600, Jens Axboe wrote:
>>>> I don't think this is the right way to fix it, ->sqe should've been
>>>> stable upfront if this ends up happening. Can you share your poc with
>>>> me? Your trace has been trimmed down way too much to be useful.
>>>
>>> Agreed that a core-level copy before the inline callback would be the
>>> right fix and would eliminate the entire class for every uring_cmd
>>> driver. The per-driver copy was meant as a minimal backportable fix
>>> for the immediate scsi_bsg path.
>>>
>>> PoC and full trace below.
>>>
>>> --- PoC (poc_bsg_toctou.c) ---
>>>
>>> Build:  gcc -O2 -pthread -static -o poc poc_bsg_toctou.c
>>> Usage:  ./poc /dev/bsg/X
>>> Needs:  2+ CPUs, io_uring, /dev/bsg/* access
>>>
>>> The racer thread flips request_len between 16 (passes the <=32 bounds
>>> check) and 128 (used by copy_from_user, overflows scmd->cmnd[32]).
>>> The overflow payload plants 0xdead000000001000 at the sense_buffer
>>> pointer offset (+84 from cmnd[0]). When scsi_queue_rq() does
>>> memset(scmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE) it faults on the
>>> corrupted pointer.
>>
>> Then the fix is to use READ_ONCE() to access the SQE fields, right?
>> Copying the entire SQE seems like unnecessary overhead. See
>> nvme_uring_cmd_io() for prior art.
> 
> That is indeed the correct fix.

To be a bit more clear for the original reporter, in the hopes that they
will send a v2. Doing things like:

	if (cmd->addr)
		validate_addr(cmd->addr);

	[...]

	Use cmd->addr, we already validated it.

Is not safe, as ->addr can change in between. All of the sqe related
bits which cmd is should follow the pattern of:

	addr = READ_ONCE(cmd->addr);
	if (addr)
		validate_addr(addr);

	[...]

	Use addr, we already validated it, and it cannot have changed.

Copying 128b in both places is a big hammer, the code just needs to use
the proper access mechanism.

-- 
Jens Axboe