* [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size
@ 2021-11-03 17:06 Tadeusz Struk
2021-11-03 17:06 ` [PATCH v2 2/2] scsi: core: remove command size deduction from scsi_setup_scsi_cmnd Tadeusz Struk
2021-11-03 17:09 ` [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size Christoph Hellwig
0 siblings, 2 replies; 7+ messages in thread
From: Tadeusz Struk @ 2021-11-03 17:06 UTC (permalink / raw)
To: Bart Van Assche, linux-scsi
Cc: Tadeusz Struk, Christoph Hellwig, James E . J . Bottomley,
Martin K . Petersen, linux-kernel, stable
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))
--
2.33.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] scsi: core: remove command size deduction from scsi_setup_scsi_cmnd
2021-11-03 17:06 [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size Tadeusz Struk
@ 2021-11-03 17:06 ` Tadeusz Struk
2021-11-03 17:17 ` Bart Van Assche
2021-11-03 17:18 ` Christoph Hellwig
2021-11-03 17:09 ` [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size Christoph Hellwig
1 sibling, 2 replies; 7+ messages in thread
From: Tadeusz Struk @ 2021-11-03 17:06 UTC (permalink / raw)
To: Bart Van Assche, linux-scsi
Cc: Tadeusz Struk, Christoph Hellwig, James E . J . Bottomley,
Martin K . Petersen, linux-kernel, stable,
syzbot+5516b30f5401d4dcbcae
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;
--
2.33.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] scsi: core: remove command size deduction from scsi_setup_scsi_cmnd
2021-11-03 17:06 ` [PATCH v2 2/2] scsi: core: remove command size deduction from scsi_setup_scsi_cmnd Tadeusz Struk
@ 2021-11-03 17:17 ` Bart Van Assche
2021-11-03 17:18 ` Christoph Hellwig
1 sibling, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2021-11-03 17:17 UTC (permalink / raw)
To: Tadeusz Struk, linux-scsi
Cc: Christoph Hellwig, James E . J . Bottomley, Martin K . Petersen,
linux-kernel, stable, syzbot+5516b30f5401d4dcbcae
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);
> cmd->cmnd = scsi_req(req)->cmd;
> cmd->transfersize = blk_rq_bytes(req);
> cmd->allowed = scsi_req(req)->retries;
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] scsi: core: remove command size deduction from scsi_setup_scsi_cmnd
2021-11-03 17:06 ` [PATCH v2 2/2] scsi: core: remove command size deduction from scsi_setup_scsi_cmnd Tadeusz Struk
2021-11-03 17:17 ` Bart Van Assche
@ 2021-11-03 17:18 ` Christoph Hellwig
1 sibling, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2021-11-03 17:18 UTC (permalink / raw)
To: Tadeusz Struk
Cc: Bart Van Assche, linux-scsi, Christoph Hellwig,
James E . J . Bottomley, Martin K . Petersen, linux-kernel,
stable, syzbot+5516b30f5401d4dcbcae
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size
2021-11-03 17:06 [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size Tadeusz Struk
2021-11-03 17:06 ` [PATCH v2 2/2] scsi: core: remove command size deduction from scsi_setup_scsi_cmnd Tadeusz Struk
@ 2021-11-03 17:09 ` Christoph Hellwig
2021-11-03 17:19 ` Tadeusz Struk
1 sibling, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2021-11-03 17:09 UTC (permalink / raw)
To: Tadeusz Struk
Cc: Bart Van Assche, linux-scsi, Christoph Hellwig,
James E . J . Bottomley, Martin K . Petersen, linux-kernel,
stable
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size
2021-11-03 17:09 ` [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size Christoph Hellwig
@ 2021-11-03 17:19 ` Tadeusz Struk
2021-11-03 17:20 ` Christoph Hellwig
0 siblings, 1 reply; 7+ messages in thread
From: Tadeusz Struk @ 2021-11-03 17:19 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Bart Van Assche, linux-scsi, James E . J . Bottomley,
Martin K . Petersen, linux-kernel, stable, Douglas Gilbert
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).
--
Thanks,
Tadeusz
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size
2021-11-03 17:19 ` Tadeusz Struk
@ 2021-11-03 17:20 ` Christoph Hellwig
0 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2021-11-03 17:20 UTC (permalink / raw)
To: Tadeusz Struk
Cc: Christoph Hellwig, Bart Van Assche, linux-scsi,
James E . J . Bottomley, Martin K . Petersen, linux-kernel,
stable, Douglas Gilbert
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-03 17:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-03 17:06 [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size Tadeusz Struk
2021-11-03 17:06 ` [PATCH v2 2/2] scsi: core: remove command size deduction from scsi_setup_scsi_cmnd Tadeusz Struk
2021-11-03 17:17 ` Bart Van Assche
2021-11-03 17:18 ` Christoph Hellwig
2021-11-03 17:09 ` [PATCH v2 1/2] scsi: scsi_ioctl: Validate command size Christoph Hellwig
2021-11-03 17:19 ` Tadeusz Struk
2021-11-03 17:20 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).