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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



From: Andrea Parri <parri.andrea@xxxxxxxxx> 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





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux