linux-kernel - RE: [PATCH] scsi: storvsc: Fix validation for unsolicited incoming packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <MWHPR21MB15933E46ABC6DC0AA5DD0A5AD7AF9@MWHPR21MB1593.namprd21.prod.outlook.com>
Date:   Tue, 5 Oct 2021 20:36:01 +0000
From:   Michael Kelley <mikelley@...rosoft.com>
To:     Andrea Parri <parri.andrea@...il.com>,
        Long Li <longli@...rosoft.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>,
        "James E . J . Bottomley" <jejb@...ux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        Dexuan Cui <decui@...rosoft.com>
Subject: RE: [PATCH] scsi: storvsc: Fix validation for unsolicited incoming
 packets

From: Andrea Parri <parri.andrea@...il.com> Sent: Tuesday, October 5, 2021 11:14 AM
> 
> > > @@ -292,6 +292,9 @@ struct vmstorage_protocol_version {
> > >  #define STORAGE_CHANNEL_REMOVABLE_FLAG		0x1
> > >  #define STORAGE_CHANNEL_EMULATED_IDE_FLAG	0x2
> > >
> > > +/* Lower bound on the size of unsolicited packets with ID of 0 */
> > > +#define VSTOR_MIN_UNSOL_PKT_SIZE		48
> > > +
> >
> > I know you have determined experimentally that Hyper-V sends
> > unsolicited packets with the above length, so the idea is to validate
> > that the guest actually gets packets at least that big.  But I wonder if
> > we should think about this slightly differently.
> >
> > The goal is for the storvsc driver to protect itself against bad or
> > malicious messages from Hyper-V.  For the unsolicited messages, the
> > only field that this storvsc driver needs to access is the
> > vstor_packet->operation field.
> 
> Eh, this is one piece of information I was looking for...  ;-)

I'm just looking at the code in storvsc_on_receive().   storvsc_on_receive()
itself looks at the "operation" field, but for the REMOVE_DEVICE and
ENUMERATE_BUS operations, you can see that the rest of the vstor_packet
is ignored and is not passed to any called functions.

> 
> 
> >So an alternate approach is to set
> > the minimum length as small as possible while ensuring that field is valid.
> 
> The fact is, I'm not sure how to do it for unsolicited messages.
> Current code ensures/checks != COMPLETE_IO.  Your comment above
> and code audit suggest that we should add a check != FCHBA_DATA.
> I saw ENUMERATE_BUS messages, code only using their "operation".

I'm not completely sure about FCHBA_DATA.  That message does not
seem to be unsolicited, as the guest sends out a message of that type in 
storvsc_channel_init() using storvsc_execute_vstor_op().  So any received
messages of that type are presumably in response to the guest request,
and will get handled via the test for rqst_id == VMBUS_RQST_INIT.  Long 
Li could probably confirm.  So if Hyper-V did send a FCHBA_DATA
packet with rqst_id of 0, it would seem to be appropriate to reject
it.

> 
> And, again, this is only based on current code/observations...
> 
> So, maybe you mean something like this (on top of this patch)?

Yes, with a comment to explain what's going on. :-)

> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 349c1071a98d4..8fedac3c7597a 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -292,9 +292,6 @@ struct vmstorage_protocol_version {
>  #define STORAGE_CHANNEL_REMOVABLE_FLAG		0x1
>  #define STORAGE_CHANNEL_EMULATED_IDE_FLAG	0x2
> 
> -/* Lower bound on the size of unsolicited packets with ID of 0 */
> -#define VSTOR_MIN_UNSOL_PKT_SIZE		48
> -
>  struct vstor_packet {
>  	/* Requested operation type */
>  	enum vstor_packet_operation operation;
> @@ -1291,7 +1288,7 @@ static void storvsc_on_channel_callback(void *context)
>  		u32 pktlen = hv_pkt_datalen(desc);
>  		u64 rqst_id = desc->trans_id;
>  		u32 minlen = rqst_id ? sizeof(struct vstor_packet) -
> -			stor_device->vmscsi_size_delta : VSTOR_MIN_UNSOL_PKT_SIZE;
> +			stor_device->vmscsi_size_delta : sizeof(enum vstor_packet_operation);
> 
>  		if (pktlen < minlen) {
>  			dev_err(&device->device,
> @@ -1315,7 +1312,8 @@ static void storvsc_on_channel_callback(void *context)
>  				 * storvsc_on_io_completion() with a guest memory address that is
>  				 * zero if Hyper-V were to construct and send such a bogus packet.
>  				 */
> -				if (packet->operation == VSTOR_OPERATION_COMPLETE_IO) {
> +				if (packet->operation == VSTOR_OPERATION_COMPLETE_IO ||
> +				    packet->operation == VSTOR_OPERATION_FCHBA_DATA) {
>  					dev_err(&device->device, "Invalid packet with ID of 0\n");
>  					continue;
>  				}
> 
> Thanks,
>   Andrea