[PATCH 1/1] scsi: Fix possible buffer overflows in storvsc_queuecommand

[PATCH 1/1] scsi: Fix possible buffer overflows in storvsc_queuecommand

[Xiaohui Zhang]

From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>

storvsc_queuecommand() calls memcpy() without checking
the destination size may trigger a buffer overflower,
which a local user could use to cause denial of service
or the execution of arbitrary code.
Fix it by putting the length check before calling memcpy().

Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
---
 drivers/scsi/storvsc_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 0c65fbd41..09b60a4c0 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1729,6 +1729,8 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd)
 
 	vm_srb->cdb_length = scmnd->cmd_len;
 
+	if (vm_srb->cdb_length > STORVSC_MAX_CMD_LEN)
+		vm_srb->cdb_length = STORVSC_MAX_CMD_LEN;
 	memcpy(vm_srb->cdb, scmnd->cmnd, vm_srb->cdb_length);
 
 	sgl = (struct scatterlist *)scsi_sglist(scmnd);
-- 
2.17.1

RE: [EXTERNAL] [PATCH 1/1] scsi: Fix possible buffer overflows in storvsc_queuecommand

[KY Srinivasan]

> -----Original Message-----
> From: Xiaohui Zhang <ruc_zhangxiaohui@163.com>
> Sent: Tuesday, December 8, 2020 5:19 AM
> To: Xiaohui Zhang <ruc_zhangxiaohui@163.com>; KY Srinivasan
> <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; Wei Liu <wei.liu@kernel.org>;
> James E.J. Bottomley <jejb@linux.ibm.com>; Martin K. Petersen
> <martin.petersen@oracle.com>; linux-hyperv@vger.kernel.org; linux-
> scsi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 1/1] scsi: Fix possible buffer overflows in
> storvsc_queuecommand
> 
> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> 
> storvsc_queuecommand() calls memcpy() without checking the destination size
> may trigger a buffer overflower, which a local user could use to cause denial of
> service or the execution of arbitrary code.
> Fix it by putting the length check before calling memcpy().
> 
> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> ---
>  drivers/scsi/storvsc_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index
> 0c65fbd41..09b60a4c0 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1729,6 +1729,8 @@ static int storvsc_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *scmnd)
> 
>  	vm_srb->cdb_length = scmnd->cmd_len;
> 
> +	if (vm_srb->cdb_length > STORVSC_MAX_CMD_LEN)
> +		vm_srb->cdb_length = STORVSC_MAX_CMD_LEN;
>  	memcpy(vm_srb->cdb, scmnd->cmnd, vm_srb->cdb_length);

The data  structure is sized correctly to handle the max command length. Besides
your check is bogus - you cannot truncate the command!

K. Y
> 
>  	sgl = (struct scatterlist *)scsi_sglist(scmnd);
> --
> 2.17.1

RE: [PATCH 1/1] scsi: Fix possible buffer overflows in storvsc_queuecommand

[Michael Kelley]

From: Xiaohui Zhang <ruc_zhangxiaohui@163.com>  Sent: Tuesday, December 8, 2020 5:19 AM
> 
> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> 
> storvsc_queuecommand() calls memcpy() without checking
> the destination size may trigger a buffer overflower,
> which a local user could use to cause denial of service
> or the execution of arbitrary code.
> Fix it by putting the length check before calling memcpy().
> 
> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> ---
>  drivers/scsi/storvsc_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 0c65fbd41..09b60a4c0 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1729,6 +1729,8 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct
> scsi_cmnd *scmnd)
> 
>  	vm_srb->cdb_length = scmnd->cmd_len;
> 
> +	if (vm_srb->cdb_length > STORVSC_MAX_CMD_LEN)
> +		vm_srb->cdb_length = STORVSC_MAX_CMD_LEN;
>  	memcpy(vm_srb->cdb, scmnd->cmnd, vm_srb->cdb_length);
> 
>  	sgl = (struct scatterlist *)scsi_sglist(scmnd);
> --
> 2.17.1

At first glance, this new test isn't necessary.  storvsc_queuecommand() gets
called from scsi_dispatch_cmd(), where just before the queuecommand function
is called, the cmd_len field is checked against the maximum command length
defined for the SCSI controller.  In the case of storvsc, that maximum command
length is STORVSC_MAX_CMD_LEN as set in storvsc_probe().  There's a comment
in scsi_dispatch_cmd() that covers this exact case.

You are correct that we need to make sure there's no buffer overflow.  Are
you seeing any other path where storvsc_queuecommand() could be called
without the cmd_len being checked?

Michael

Re: [PATCH 1/1] scsi: Fix possible buffer overflows in storvsc_queuecommand

[Wei Liu]

On Wed, Dec 09, 2020 at 09:25:23AM +0800, Xiaohui Zhang wrote:
> 
> 
> 
> At 2020-12-09 01:52:42, "Michael Kelley" <mikelley@microsoft.com> wrote:
> >From: Xiaohui Zhang <ruc_zhangxiaohui@163.com>  Sent: Tuesday, December 8, 2020 5:19 AM
> >> 
> >> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> >> 
> >> storvsc_queuecommand() calls memcpy() without checking
> >> the destination size may trigger a buffer overflower,
> >> which a local user could use to cause denial of service
> >> or the execution of arbitrary code.
> >> Fix it by putting the length check before calling memcpy().
> >> 
> >> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> >> ---
> >>  drivers/scsi/storvsc_drv.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> >> index 0c65fbd41..09b60a4c0 100644
> >> --- a/drivers/scsi/storvsc_drv.c
> >> +++ b/drivers/scsi/storvsc_drv.c
> >> @@ -1729,6 +1729,8 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct
> >> scsi_cmnd *scmnd)
> >> 
> >>  	vm_srb->cdb_length = scmnd->cmd_len;
> >> 
> >> +	if (vm_srb->cdb_length > STORVSC_MAX_CMD_LEN)
> >> +		vm_srb->cdb_length = STORVSC_MAX_CMD_LEN;
> >>  	memcpy(vm_srb->cdb, scmnd->cmnd, vm_srb->cdb_length);
> >> 
> >>  	sgl = (struct scatterlist *)scsi_sglist(scmnd);
> >> --
> >> 2.17.1
> >
> >At first glance, this new test isn't necessary.  storvsc_queuecommand() gets
> >called from scsi_dispatch_cmd(), where just before the queuecommand function
> >is called, the cmd_len field is checked against the maximum command length
> >defined for the SCSI controller.  In the case of storvsc, that maximum command
> >length is STORVSC_MAX_CMD_LEN as set in storvsc_probe().  There's a comment
> >in scsi_dispatch_cmd() that covers this exact case.
> >
> >You are correct that we need to make sure there's no buffer overflow.  Are
> >you seeing any other path where storvsc_queuecommand() could be called
> 
> >without the cmd_len being checked?
> 
> 
> Hello, maybe storvsc_queuecommand() could be called without the cmd_len 
> being checked in scsi_send_eh_cmnd().

In that case, a better approach is to fix the SCSI layer instead of
individual drivers, right? Storvsc can't be the only one that is
affected by this issue.

Truncating the command solves the buffer overflow issue but it doesn't
make sense to issue a truncated command to the controller.

Wei.