Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Dec 2020 13:31:46 +0000
From:   Wei Liu <wei.liu@kernel.org>
To:     Xiaohui Zhang <ruc_zhangxiaohui@163.com>
Cc:     Michael Kelley <mikelley@microsoft.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-hyperv@vger.kernel.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/1] scsi: Fix possible buffer overflows in
 storvsc_queuecommand
Message-ID: <20201209133146.yskq5rnxpdaiqhrc@liuwe-devbox-debian-v2>
References: <20201208131918.31534-1-ruc_zhangxiaohui@163.com>
 <MW2PR2101MB10523F50AA4497A9E116FBC0D7CD1@MW2PR2101MB1052.namprd21.prod.outlook.com>
 <56cde2c6.bb9.176451a57d7.Coremail.ruc_zhangxiaohui@163.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <56cde2c6.bb9.176451a57d7.Coremail.ruc_zhangxiaohui@163.com>
User-Agent: NeoMutt/20180716
Precedence: bulk

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.