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

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

[Yang Xiuwei]

IO_URING_F_NONBLOCK is only meant to make request allocation
non-blocking via BLK_MQ_REQ_NOWAIT. Propagating it
to blk_rq_map_user() by using GFP_NOWAIT for bio allocation
is a separate, stricter limitation. bio_alloc_bioset() gives up
without using the bio mempool when GFP_NOWAIT is passed and
the initial slab allocation fails. That can cause user
buffer mapping to fail with -ENOMEM even when memory and
request tags are otherwise available, which is not what NONBLOCK
submission is supposed to mean.

Always map uring_cmd user buffers with GFP_KERNEL and keep NOWAIT
limited to scsi_alloc_request().

Fixes: 7b6d3255e7f8 ("scsi: bsg: add io_uring passthrough handler")
Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
---
 drivers/scsi/scsi_bsg.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_bsg.c b/drivers/scsi/scsi_bsg.c
index e80dec53174e..0fdc13d67c89 100644
--- a/drivers/scsi/scsi_bsg.c
+++ b/drivers/scsi/scsi_bsg.c
@@ -76,7 +76,7 @@ static enum rq_end_io_ret scsi_bsg_uring_cmd_done(struct request *req,
 
 static int scsi_bsg_map_user_buffer(struct request *req,
 				    struct io_uring_cmd *ioucmd,
-				    unsigned int issue_flags, gfp_t gfp_mask)
+				    unsigned int issue_flags)
 {
 	const struct bsg_uring_cmd *cmd = io_uring_sqe128_cmd(ioucmd->sqe, struct bsg_uring_cmd);
 	bool is_write = cmd->dout_xfer_len > 0;
@@ -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);
 	}
 
 	return ret;
@@ -108,7 +108,6 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
 	struct scsi_cmnd *scmd;
 	struct request *req;
 	blk_mq_req_flags_t blk_flags = 0;
-	gfp_t gfp_mask = GFP_KERNEL;
 	int ret;
 
 	if (cmd->protocol != BSG_PROTOCOL_SCSI ||
@@ -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);
@@ -159,7 +156,7 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
 		min(cmd->max_response_len, SCSI_SENSE_BUFFERSIZE) : SCSI_SENSE_BUFFERSIZE;
 
 	if (cmd->dout_xfer_len || cmd->din_xfer_len) {
-		ret = scsi_bsg_map_user_buffer(req, ioucmd, issue_flags, gfp_mask);
+		ret = scsi_bsg_map_user_buffer(req, ioucmd, issue_flags);
 		if (ret)
 			goto out_free_req;
 		pdu->bio = req->bio;
-- 
2.25.1

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

[sashiko-bot]

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