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

The validation on the length of incoming packets performed in
storvsc_on_channel_callback() does not apply to unsolicited
packets with ID of 0 sent by Hyper-V.  Adjust the validation
for such unsolicited packets.

Fixes: 91b1b640b834b2 ("scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()")
Reported-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
Changes since RFC[1]:
  - Merge length checks (Haiyang Zhang)

[1] https://lkml.kernel.org/r/20210928163732.5908-1-parri.andrea@gmail.com

 drivers/scsi/storvsc_drv.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index ebbbc1299c625..349c1071a98d4 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c

@@ -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
+
 struct vstor_packet {
 	/* Requested operation type */
 	enum vstor_packet_operation operation;

@@ -1285,11 +1288,15 @@ static void storvsc_on_channel_callback(void *context)
 	foreach_vmbus_pkt(desc, channel) {
 		struct vstor_packet *packet = hv_pkt_data(desc);
 		struct storvsc_cmd_request *request = NULL;
+		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;
 
-		if (hv_pkt_datalen(desc) < sizeof(struct vstor_packet) -
-				stor_device->vmscsi_size_delta) {
-			dev_err(&device->device, "Invalid packet len\n");
+		if (pktlen < minlen) {
+			dev_err(&device->device,
+				"Invalid pkt: id=%llu, len=%u, minlen=%u\n",
+				rqst_id, pktlen, minlen);
 			continue;
 		}
 

-- 
2.25.1

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Tuesday, October 5, 2021 4:41 AM

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.  So an alternate approach is to set
the minimum length as small as possible while ensuring that field is valid.
Then if Hyper-V later changes the size of these unsolicited packets to
some smaller size that still contains a valid "operation" field, this code
will still work.   If in a new version of the protocol Hyper-V adds fields
that this driver needs to look at, then the minimum size can be
adjusted as needed for that new protocol version.

I'm good with the rest of the code.  It's just a question of whether to perhaps
"future-proof" the code by not requiring a packet size any bigger than the
driver actually needs to reference.

Michael

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

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

quoted

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

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.

quoted

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

My (current) best guess is here:

  https://lkml.kernel.org/r/20211006132026.4089-1-parri.andrea@gmail.com

Thanks,
  Andrea