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

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...  ;-)

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".

And, again, this is only based on current code/observations...

So, maybe you mean something like this (on top of this patch)?

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