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

On Sun, Jul 19, 2026 at 8:27 PM Yang Xiuwei <[email protected]> wrote:
>
> From: Rahul Chandelkar <[email protected]>
>
> 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: [email protected]
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Rahul Chandelkar <[email protected]>
> Co-developed-by: Yang Xiuwei <[email protected]>
> Signed-off-by: Yang Xiuwei <[email protected]>
> ---
>  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;

cmd->max_response_len is passed through here, shouldn't it be accessed
using READ_ONCE() too? Omitting READ_ONCE() only seems safe for values
that are ignored (except for a possible check that they match a fixed
value).

Ditto for cmd->timeout_ms later in this function.

Best,
Caleb

>
> -       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
>
>