RE: [PATCH] scsi: storvsc: Fix validation for unsolicited incoming packets
From: Michael Kelley <hidden>
Date: 2021-10-05 20:36:09
Also in:
linux-scsi, lkml
From: Andrea Parri <parri.andrea@gmail.com> Sent: Tuesday, October 5, 2021 11:14 AM
quoted
quoted
@@ -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.
quoted
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. :-)
quoted hunk ↗ jump to hunk
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