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

On Wed, Dec 09, 2020 at 09:25:23AM +0800, Xiaohui Zhang wrote:

At 2020-12-09 01:52:42, "Michael Kelley" [off-list ref] wrote:

quoted

From: Xiaohui Zhang <redacted>  Sent: Tuesday, December 8, 2020 5:19 AM

quoted

From: Zhang Xiaohui <redacted>

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

quoted

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.