Re: [PATCH] scsi: bsg: do not use GFP_NOWAIT for uring_cmd user buffer mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unconditional use of GFP_KERNEL in a non-blocking io_uring context violates non-blocking semantics and causes task_work deadlocks.

Pre-existing issues:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability due to multiple direct reads of SQE fields (`cmd->request_len`, `cmd->dout_xfer_len`, etc.) from userspace memory, leading to a heap buffer overflow.
- [High] Allocating memory with GFP_KERNEL while holding a block request tag can cause a deadlock under memory pressure.
--

commit f109aac7c9c45c1c7f2d18b3b1be8bc0e39e4c6e
Author: Yang Xiuwei <yangxiuwei@kylinos.cn>

scsi: bsg: do not use GFP_NOWAIT for uring_cmd user buffer mapping

This commit changes the bio allocation in the io_uring passthrough handler
to unconditionally use GFP_KERNEL instead of honoring the NOWAIT flag,
limiting NOWAIT to the block request allocation only.

> --- a/drivers/scsi/scsi_bsg.c
> +++ b/drivers/scsi/scsi_bsg.c
> @@ -91,10 +91,10 @@ static int scsi_bsg_map_user_buffer(struct request *req,
>  						&iter, ioucmd, issue_flags);
>  		if (ret < 0)
>  			return ret;
> -		ret = blk_rq_map_user_iov(req->q, req, NULL, &iter, gfp_mask);
> +		ret = blk_rq_map_user_iov(req->q, req, NULL, &iter, GFP_KERNEL);
>  	} else {
>  		ret = blk_rq_map_user(req->q, req, NULL, uptr64(buf_addr),
> -				      buf_len, gfp_mask);
> +				      buf_len, GFP_KERNEL);
>  	}

[Severity: High]
When mapping user buffers unconditionally with GFP_KERNEL here, does this
violate non-blocking semantics for io_uring?

If the mempool is depleted, mempool_alloc can sleep the submitter thread
indefinitely waiting for a bio. Because bios are freed via task_work by the
submitter task, blocking the submitter here prevents task_work from
executing, creating a circular deadlock.

Should this attempt allocation with GFP_NOWAIT first, and return -EAGAIN on
failure to trigger an io-wq punt?

[ ... ]

> @@ -126,10 +125,8 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
>  	if (cmd->dout_iovec_count > 0 || cmd->din_iovec_count > 0)
>  		return -EOPNOTSUPP;
>  
> -	if (issue_flags & IO_URING_F_NONBLOCK) {
> +	if (issue_flags & IO_URING_F_NONBLOCK)
>  		blk_flags = BLK_MQ_REQ_NOWAIT;
> -		gfp_mask = GFP_NOWAIT;
> -	}
>  
>  	req = scsi_alloc_request(q, cmd->dout_xfer_len ?
>  				 REQ_OP_DRV_OUT : REQ_OP_DRV_IN, blk_flags);

[Severity: High]
This is a pre-existing issue, but does performing memory allocation with
GFP_KERNEL while holding a block request tag risk deadlock under memory
pressure?

In scsi_bsg_uring_cmd(), scsi_alloc_request() consumes a hardware tag
from the shared tag set. Subsequently calling scsi_bsg_map_user_buffer()
with GFP_KERNEL can enter direct reclaim.

If memory reclaim needs to write dirty pages back to a block device on the
same host, it requires a request tag. If all tags are held by tasks
suspended in GFP_KERNEL, memory reclaim cannot make progress.

[Severity: Critical]
This is a pre-existing issue, but is there a Time-of-Check to
Time-of-Use (TOCTOU) vulnerability here?

The cmd->request_len field is read directly from the userspace-mapped
submission queue entry (ioucmd->sqe). A malicious userspace program
could mutate the SQE after the bounds check:

scsi_bsg_uring_cmd() {
    ...
    if (cmd->request_len > sizeof(scmd->cmnd)) {
        ret = -EINVAL;
        goto out_free_req;
    }

but before the copy executes:

    if (copy_from_user(scmd->cmnd, uptr64(cmd->request), cmd->request_len)) {
        ret = -EFAULT;
    ...
}

Could this allow copy_from_user to overflow the fixed 32-byte scmd->cmnd
buffer in the request PDU? The io_uring subsystem guidelines mandate
caching SQE fields locally via READ_ONCE() before validation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619013833.2312908-1-yangxiuwei@kylinos.cn?part=1