[PATCH 0/2] scsi: bsg: fix io_uring passthrough issues

[PATCH 0/2] scsi: bsg: fix io_uring passthrough issues

[Yang Xiuwei]

Hi,

Two fixes for the BSG io_uring passthrough path.

Patch 1 addresses a TOCTOU where the shared SQE can change request_len
between validation and copy_from_user(), overflowing scmd->cmnd. This
is a v3 of Rahul Chandelkar's series, taking over after no follow-up to
v2. Relative to v2, READ_ONCE is limited to fields used for validation
and buffer mapping, as requested in review.

Link: https://lore.kernel.org/r/20260527105931.3950913-1-rc@rexion.ai
Link: https://lore.kernel.org/r/20260527191817.142769-1-rc@rexion.ai

Patch 2 uses GFP_KERNEL for user-buffer mapping. IO_URING_F_NONBLOCK
still sets BLK_MQ_REQ_NOWAIT so request-tag allocation does not block;
mapping itself may sleep and need not use GFP_NOWAIT.

Changes for patch 1 since Rahul's v2:
- READ_ONCE only request/request_len and dout/din xfer fields.
- Pass stable map arguments into scsi_bsg_map_user_buffer().

Please review.

Thanks,
Yang Xiuwei

Rahul Chandelkar (1):
  scsi: bsg: fix TOCTOU in io_uring passthrough command setup

Yang Xiuwei (1):
  scsi: bsg: map io_uring user buffers with GFP_KERNEL

 drivers/scsi/scsi_bsg.c | 47 +++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH 1/2] scsi: bsg: fix TOCTOU in io_uring passthrough command setup

[Yang Xiuwei]

From: Rahul Chandelkar <rc@rexion.ai>

scsi_bsg_uring_cmd() reads bsg_uring_cmd fields from the shared mmap'd
SQE via io_uring_sqe128_cmd(). On the inline path the SQE is still
user-writable, so request_len can change between the bounds check and
copy_from_user(), overflowing scmd->cmnd.

Snapshot request/request_len and the transfer fields used for buffer
mapping with READ_ONCE before validation and reuse. Pass the stable
map arguments into scsi_bsg_map_user_buffer() so it does not re-read
the SQE.

Fixes: 7b6d3255e7f8 ("scsi: bsg: add io_uring passthrough handler")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20260527105931.3950913-1-rc@rexion.ai
Signed-off-by: Rahul Chandelkar <rc@rexion.ai>
Co-developed-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
---
 drivers/scsi/scsi_bsg.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_bsg.c b/drivers/scsi/scsi_bsg.c
index e80dec53174e..c57ce01379de 100644
--- a/drivers/scsi/scsi_bsg.c
+++ b/drivers/scsi/scsi_bsg.c
@@ -76,12 +76,10 @@ 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, gfp_t gfp_mask,
+				    bool is_write, u64 buf_addr,
+				    unsigned long buf_len)
 {
-	const struct bsg_uring_cmd *cmd = io_uring_sqe128_cmd(ioucmd->sqe, struct bsg_uring_cmd);
-	bool is_write = cmd->dout_xfer_len > 0;
-	u64 buf_addr = is_write ? cmd->dout_xferp : cmd->din_xferp;
-	unsigned long buf_len = is_write ? cmd->dout_xfer_len : cmd->din_xfer_len;
 	struct iov_iter iter;
 	int ret;
 
@@ -104,21 +102,29 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
 			       unsigned int issue_flags, bool open_for_write)
 {
 	struct scsi_bsg_uring_cmd_pdu *pdu = scsi_bsg_uring_cmd_pdu(ioucmd);
-	const struct bsg_uring_cmd *cmd = io_uring_sqe128_cmd(ioucmd->sqe, struct bsg_uring_cmd);
+	const struct bsg_uring_cmd *cmd =
+		io_uring_sqe128_cmd(ioucmd->sqe, struct bsg_uring_cmd);
 	struct scsi_cmnd *scmd;
 	struct request *req;
 	blk_mq_req_flags_t blk_flags = 0;
 	gfp_t gfp_mask = GFP_KERNEL;
+	/* Snapshot SQE fields used for validation and buffer mapping. */
+	u64 request = READ_ONCE(cmd->request);
+	u32 request_len = READ_ONCE(cmd->request_len);
+	u64 dout_xferp = READ_ONCE(cmd->dout_xferp);
+	u32 dout_xfer_len = READ_ONCE(cmd->dout_xfer_len);
+	u64 din_xferp = READ_ONCE(cmd->din_xferp);
+	u32 din_xfer_len = READ_ONCE(cmd->din_xfer_len);
 	int ret;
 
 	if (cmd->protocol != BSG_PROTOCOL_SCSI ||
 	    cmd->subprotocol != BSG_SUB_PROTOCOL_SCSI_CMD)
 		return -EINVAL;
 
-	if (!cmd->request || cmd->request_len == 0)
+	if (!request || request_len == 0)
 		return -EINVAL;
 
-	if (cmd->dout_xfer_len && cmd->din_xfer_len) {
+	if (dout_xfer_len && din_xfer_len) {
 		pr_warn_once("BIDI support in bsg has been removed.\n");
 		return -EOPNOTSUPP;
 	}
@@ -131,20 +137,20 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
 		gfp_mask = GFP_NOWAIT;
 	}
 
-	req = scsi_alloc_request(q, cmd->dout_xfer_len ?
+	req = scsi_alloc_request(q, dout_xfer_len ?
 				 REQ_OP_DRV_OUT : REQ_OP_DRV_IN, blk_flags);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
 	scmd = blk_mq_rq_to_pdu(req);
-	if (cmd->request_len > sizeof(scmd->cmnd)) {
+	if (request_len > sizeof(scmd->cmnd)) {
 		ret = -EINVAL;
 		goto out_free_req;
 	}
-	scmd->cmd_len = cmd->request_len;
+	scmd->cmd_len = request_len;
 	scmd->allowed = SG_DEFAULT_RETRIES;
 
-	if (copy_from_user(scmd->cmnd, uptr64(cmd->request), cmd->request_len)) {
+	if (copy_from_user(scmd->cmnd, uptr64(request), request_len)) {
 		ret = -EFAULT;
 		goto out_free_req;
 	}
@@ -158,8 +164,14 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
 	scmd->sense_len = cmd->max_response_len ?
 		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);
+	if (dout_xfer_len || din_xfer_len) {
+		bool is_write = dout_xfer_len > 0;
+		u64 buf_addr = is_write ? dout_xferp : din_xferp;
+		unsigned long buf_len = is_write ? dout_xfer_len : din_xfer_len;
+
+		ret = scsi_bsg_map_user_buffer(req, ioucmd, issue_flags,
+					       gfp_mask, is_write, buf_addr,
+					       buf_len);
 		if (ret)
 			goto out_free_req;
 		pdu->bio = req->bio;
-- 
2.25.1

[PATCH 2/2] scsi: bsg: map io_uring user buffers with GFP_KERNEL

[Yang Xiuwei]

IO_URING_F_NONBLOCK only needs BLK_MQ_REQ_NOWAIT for request
allocation. Mapping the user buffer may sleep, so use GFP_KERNEL
instead of GFP_NOWAIT.

Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
---
 drivers/scsi/scsi_bsg.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_bsg.c b/drivers/scsi/scsi_bsg.c
index c57ce01379de..f90d19fdbd8e 100644
--- a/drivers/scsi/scsi_bsg.c
+++ b/drivers/scsi/scsi_bsg.c
@@ -76,9 +76,8 @@ 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,
-				    bool is_write, u64 buf_addr,
-				    unsigned long buf_len)
+				    unsigned int issue_flags, bool is_write,
+				    u64 buf_addr, unsigned long buf_len)
 {
 	struct iov_iter iter;
 	int ret;
@@ -89,10 +88,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;
@@ -107,7 +106,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;
 	/* Snapshot SQE fields used for validation and buffer mapping. */
 	u64 request = READ_ONCE(cmd->request);
 	u32 request_len = READ_ONCE(cmd->request_len);
@@ -132,10 +130,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, dout_xfer_len ?
 				 REQ_OP_DRV_OUT : REQ_OP_DRV_IN, blk_flags);
@@ -170,8 +166,7 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
 		unsigned long buf_len = is_write ? dout_xfer_len : din_xfer_len;
 
 		ret = scsi_bsg_map_user_buffer(req, ioucmd, issue_flags,
-					       gfp_mask, is_write, buf_addr,
-					       buf_len);
+					       is_write, buf_addr, buf_len);
 		if (ret)
 			goto out_free_req;
 		pdu->bio = req->bio;
-- 
2.25.1

Re: [PATCH 1/2] scsi: bsg: fix TOCTOU in io_uring passthrough command setup

[sashiko-bot]

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

Pre-existing issues:
- [High] Kernel memory leak of uninitialized sense buffer bytes to user space.
--

--- Patch [1]: [PATCH 1/2] scsi: bsg: fix TOCTOU in io_uring passthrough command setup ---
commit bb53b6e06613a855cd8556487582a73b36038ec3
Author: Rahul Chandelkar <rc@rexion.ai>

scsi: bsg: fix TOCTOU in io_uring passthrough command setup

This commit fixes a TOCTOU vulnerability in scsi_bsg_uring_cmd() by snapshotting
SQE fields with READ_ONCE() before validation and reuse. The stable map
arguments are then passed into scsi_bsg_map_user_buffer().

Link: https://lore.kernel.org/r/20260527105931.3950913-1-rc@rexion.ai

> diff --git a/drivers/scsi/scsi_bsg.c b/drivers/scsi/scsi_bsg.c
> index e80dec53174ea..c57ce01379de5 100644
> --- a/drivers/scsi/scsi_bsg.c
> +++ b/drivers/scsi/scsi_bsg.c
[ ... ]
> @@ -158,8 +164,14 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
>  	scmd->sense_len = cmd->max_response_len ?
>  		min(cmd->max_response_len, SCSI_SENSE_BUFFERSIZE) : SCSI_SENSE_BUFFERSIZE;
>  

[Severity: High]
This isn't a bug introduced by this patch, but does overwriting scmd->sense_len
with the maximum response size leak uninitialized kernel memory to user space?

The SCSI midlayer expects sense_len to hold the actual valid length of the
sense data, but it is overwritten here with the user's max_response_len.

If a command fails, the low-level driver might populate a small amount of sense
data (e.g., 18 bytes) without zeroing the rest of the buffer.

The completion handler in drivers/scsi/scsi_bsg.c retrieves this artificially
inflated sense_len to determine how many bytes to copy:

scsi_bsg_uring_task_cb() {
    ...
    if (pdu->response_addr)
        sense_len_wr = min_t(u8, scmd->sense_len,
                             SCSI_SENSE_BUFFERSIZE);
    ...
    if (copy_to_user(uptr64(pdu->response_addr), scmd->sense_buffer,
                     sense_len_wr))
    ...
}

Could this copy uninitialized trailing bytes from the reused sense_buffer to
user space?

> -	if (cmd->dout_xfer_len || cmd->din_xfer_len) {
> -		ret = scsi_bsg_map_user_buffer(req, ioucmd, issue_flags, gfp_mask);
> +	if (dout_xfer_len || din_xfer_len) {
> +		bool is_write = dout_xfer_len > 0;
> +		u64 buf_addr = is_write ? dout_xferp : din_xferp;
> +		unsigned long buf_len = is_write ? dout_xfer_len : din_xfer_len;
> +
> +		ret = scsi_bsg_map_user_buffer(req, ioucmd, issue_flags,
> +					       gfp_mask, is_write, buf_addr,
> +					       buf_len);

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

Re: [PATCH 2/2] scsi: bsg: map io_uring user buffers with GFP_KERNEL

[sashiko-bot]

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

New issues:
- [High] Unconditionally using GFP_KERNEL forces the io_uring submitter thread to sleep, breaking non-blocking semantics.

Pre-existing issues:
- [High] Using GFP_KERNEL while holding a block layer request tag introduces a tag exhaustion deadlock during memory reclaim.
--

--- Patch [2]: [PATCH 2/2] scsi: bsg: map io_uring user buffers with GFP_KERNEL ---
commit e89699f2c58014fd3728ce2e8bed2b71fc1c0abf
Author: Yang Xiuwei <yangxiuwei@kylinos.cn>

scsi: bsg: map io_uring user buffers with GFP_KERNEL

IO_URING_F_NONBLOCK only needs BLK_MQ_REQ_NOWAIT for request
allocation. Mapping the user buffer may sleep, so use GFP_KERNEL
instead of GFP_NOWAIT.

> diff --git a/drivers/scsi/scsi_bsg.c b/drivers/scsi/scsi_bsg.c
> index c57ce01379de5..f90d19fdbd8ea 100644
> --- a/drivers/scsi/scsi_bsg.c
> +++ b/drivers/scsi/scsi_bsg.c
> @@ -76,9 +76,8 @@ 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,
> -				    bool is_write, u64 buf_addr,
> -				    unsigned long buf_len)
> +				    unsigned int issue_flags, bool is_write,
> +				    u64 buf_addr, unsigned long buf_len)
>  {
>  	struct iov_iter iter;
>  	int ret;
> @@ -89,10 +88,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);

[Severity: High]
This is a pre-existing issue, but does using GFP_KERNEL here while holding a
block request tag risk a tag exhaustion deadlock?

The request tag is allocated in scsi_alloc_request() prior to this mapping
step. If blk_rq_map_user_iov() triggers direct memory reclaim, and reclaim
attempts to write dirty pages to this same block device, it will need a tag.
If all tags are currently held by concurrent tasks stuck in this mapping
phase, won't the writeback block indefinitely?

>  	} else {
>  		ret = blk_rq_map_user(req->q, req, NULL, uptr64(buf_addr),
> -				      buf_len, gfp_mask);
> +				      buf_len, GFP_KERNEL);
>  	}
>  
>  	return ret;

[ ... ]

> @@ -132,10 +130,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, dout_xfer_len ?
>  				 REQ_OP_DRV_OUT : REQ_OP_DRV_IN, blk_flags);

[ ... ]

> @@ -170,8 +166,7 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
>  		unsigned long buf_len = is_write ? dout_xfer_len : din_xfer_len;
>  
>  		ret = scsi_bsg_map_user_buffer(req, ioucmd, issue_flags,
> -					       gfp_mask, is_write, buf_addr,
> -					       buf_len);
> +					       is_write, buf_addr, buf_len);

[Severity: High]
Since this changes the allocation to GFP_KERNEL unconditionally, won't this
illegally block the io_uring submission context when IO_URING_F_NONBLOCK is
set?

If a non-blocking context like SQPOLL is forced to block for memory
allocation or page faults during scsi_bsg_map_user_buffer(), doesn't it stall
all other rings sharing the thread?

Should this path attempt mapping with GFP_NOWAIT first, and return -EAGAIN on
-ENOMEM so the operation can be safely punted to an async io-wq worker?

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