Need to make sure the command size is valid before copying the command from user.
Cc: Bart Van Assche bvanassche@acm.org Cc: Christoph Hellwig hch@lst.de Cc: James E.J. Bottomley jejb@linux.ibm.com Cc: Martin K. Petersen martin.petersen@oracle.com Cc: linux-scsi@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org # 5.15, 5.14, 5.10 Signed-off-by: Tadeusz Struk tadeusz.struk@linaro.org --- Changes in v2: - removed check for upper len limit as it is handled in sg_io() --- drivers/scsi/scsi_ioctl.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c index 6ff2207bd45a..a06c61f22742 100644 --- a/drivers/scsi/scsi_ioctl.c +++ b/drivers/scsi/scsi_ioctl.c @@ -347,6 +347,8 @@ static int scsi_fill_sghdr_rq(struct scsi_device *sdev, struct request *rq, { struct scsi_request *req = scsi_req(rq);
+ if (hdr->cmd_len < 6) + return -EMSGSIZE; if (copy_from_user(req->cmd, hdr->cmdp, hdr->cmd_len)) return -EFAULT; if (!scsi_cmd_allowed(req->cmd, mode))
No need to deduce command size in scsi_setup_scsi_cmnd() anymore as appropriate checks have been added to scsi_fill_sghdr_rq() function and the cmd_len should never be zero here. The code to do that wasn't correct anyway, as it used uninitialized cmd->cmnd, which caused a null-ptr-deref if the command size was zero as in the trace below. Fix this by removing the unneeded code.
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] CPU: 0 PID: 1822 Comm: repro Not tainted 5.15.0 #1 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-4.fc34 04/01/2014 Call Trace: blk_mq_dispatch_rq_list+0x7c7/0x12d0 __blk_mq_sched_dispatch_requests+0x244/0x380 blk_mq_sched_dispatch_requests+0xf0/0x160 __blk_mq_run_hw_queue+0xe8/0x160 __blk_mq_delay_run_hw_queue+0x252/0x5d0 blk_mq_run_hw_queue+0x1dd/0x3b0 blk_mq_sched_insert_request+0x1ff/0x3e0 blk_execute_rq_nowait+0x173/0x1e0 blk_execute_rq+0x15c/0x540 sg_io+0x97c/0x1370 scsi_ioctl+0xe16/0x28e0 sd_ioctl+0x134/0x170 blkdev_ioctl+0x362/0x6e0 block_ioctl+0xb0/0xf0 vfs_ioctl+0xa7/0xf0 do_syscall_64+0x3d/0xb0 entry_SYSCALL_64_after_hwframe+0x44/0xae ---[ end trace 8b086e334adef6d2 ]--- Kernel panic - not syncing: Fatal exception
Cc: Bart Van Assche bvanassche@acm.org Cc: Christoph Hellwig hch@lst.de Cc: James E.J. Bottomley jejb@linux.ibm.com Cc: Martin K. Petersen martin.petersen@oracle.com Cc: linux-scsi@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org # 5.15, 5.14, 5.10 Fixes: 2ceda20f0a99a74a82b78870f3b3e5fa93087a7f Reported-by: syzbot+5516b30f5401d4dcbcae@syzkaller.appspotmail.com Signed-off-by: Tadeusz Struk tadeusz.struk@linaro.org --- Changes in v2: - prune trace dump according to feedback --- drivers/scsi/scsi_lib.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 572673873ddf..e026da7549be 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1174,8 +1174,6 @@ static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev, }
cmd->cmd_len = scsi_req(req)->cmd_len; - if (cmd->cmd_len == 0) - cmd->cmd_len = scsi_command_size(cmd->cmnd); cmd->cmnd = scsi_req(req)->cmd; cmd->transfersize = blk_rq_bytes(req); cmd->allowed = scsi_req(req)->retries;
On 11/3/21 10:06 AM, Tadeusz Struk wrote:
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 572673873ddf..e026da7549be 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1174,8 +1174,6 @@ static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev, } cmd->cmd_len = scsi_req(req)->cmd_len;
- if (cmd->cmd_len == 0)
cmd->cmd_len = scsi_command_size(cmd->cmnd);
Reviewed-by: Bart Van Assche bvanassche@acm.org
Looks good,
Reviewed-by: Christoph Hellwig hch@lst.de
On Wed, Nov 03, 2021 at 10:06:58AM -0700, Tadeusz Struk wrote:
Need to make sure the command size is valid before copying the command from user.
Cc: Bart Van Assche bvanassche@acm.org Cc: Christoph Hellwig hch@lst.de Cc: James E.J. Bottomley jejb@linux.ibm.com Cc: Martin K. Petersen martin.petersen@oracle.com Cc: linux-scsi@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org # 5.15, 5.14, 5.10 Signed-off-by: Tadeusz Struk tadeusz.struk@linaro.org
Changes in v2:
- removed check for upper len limit as it is handled in sg_io()
drivers/scsi/scsi_ioctl.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c index 6ff2207bd45a..a06c61f22742 100644 --- a/drivers/scsi/scsi_ioctl.c +++ b/drivers/scsi/scsi_ioctl.c @@ -347,6 +347,8 @@ static int scsi_fill_sghdr_rq(struct scsi_device *sdev, struct request *rq, { struct scsi_request *req = scsi_req(rq);
- if (hdr->cmd_len < 6)
return -EMSGSIZE;
The checks looks good, but I'd be tempted to place it next to the other check on hdr->cmd_len in the caller.
On 11/3/21 10:09, Christoph Hellwig wrote:
- if (hdr->cmd_len < 6)
return -EMSGSIZE;The checks looks good, but I'd be tempted to place it next to the other check on hdr->cmd_len in the caller.
Do you mean in sg_io()? I don't mind changing it, but putting the check here in scsi_fill_sghdr_rq() was suggested by Douglas (cc'ed now).
On Wed, Nov 03, 2021 at 10:19:21AM -0700, Tadeusz Struk wrote:
On 11/3/21 10:09, Christoph Hellwig wrote:
- if (hdr->cmd_len < 6)
return -EMSGSIZE;The checks looks good, but I'd be tempted to place it next to the other check on hdr->cmd_len in the caller.
Do you mean in sg_io()? I don't mind changing it, but putting the check here in scsi_fill_sghdr_rq() was suggested by Douglas (cc'ed now).
Ok, let's keep it that way for now.
linux-stable-mirror@lists.linaro.org
-
Bart Van Assche -
Christoph Hellwig -
Tadeusz Struk