RE: [PATCH 1/1] scsi: Fix possible buffer overflows in storvsc_queuecommand
From: Michael Kelley <hidden>
Date: 2020-12-08 17:53:36
Also in:
linux-scsi, lkml
From: Xiaohui Zhang <redacted> Sent: Tuesday, December 8, 2020 5:19 AM
quoted hunk ↗ jump to hunk
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, structscsi_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