From: Andrea Parri <parri.andrea@gmail.com> Sent: Wednesday, October 6, 2021 9:18 AM

quoted

quoted

@@ -1302,13 +1306,25 @@ static void storvsc_on_channel_callback(void *context)
 			if (rqst_id == 0) {
 				/*
 				 * storvsc_on_receive() looks at the vstor_packet in the message
-				 * from the ring buffer.  If the operation in the vstor_packet is
-				 * COMPLETE_IO, then we call storvsc_on_io_completion(), and
-				 * dereference the guest memory address.  Make sure we don't call
-				 * storvsc_on_io_completion() with a guest memory address that is
-				 * zero if Hyper-V were to construct and send such a bogus packet.
+				 * from the ring buffer.
+				 *
+				 * - If the operation in the vstor_packet is COMPLETE_IO, then
+				 *   we call storvsc_on_io_completion(), and dereference the
+				 *   guest memory address.  Make sure we don't call
+				 *   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 the operation in the vstor_packet is FCHBA_DATA, then
+				 *   we call cache_wwn(), and access the data payload area of
+				 *   the packet (wwn_packet); however, there is no guarantee
+				 *   that the packet is big enough to contain such area.
+				 *   Future-proof the code by rejecting such a bogus packet.

The comments look good to me.

quoted

+				 *
+				 * XXX.  Filter out all "invalid" operations.

Is this a leftover comment line that should be deleted?  I'm not sure about the "XXX".

That was/is intended as a "TODO".  What I think we are missing here is a
specification/authority stating "allowed vstor_operation for unsolicited
messages are: ENUMERATE_BUS, REMOVE_DEVICE, etc.".  If we wanted to make
this code even more "future-proof"/"robust", we would reject all packets
whose "operation" doesn't match that list (independently from the actual
form/implementation of storvsc_on_receive()...).  We are not quite there
tough AFAICT.

Hmmm.  I think maybe we *are* there. :-)   If we get a packet with rqst_id
of zero and a vstor operation other than COMPLETE_IO or FCHBA_DATA,
then storvsc_on_receive() will be called.  The vstor operation will be
checked there, and anything not listed in the switch statement is silently
ignored, which I think is good enough.  We could output a message
in the "default" leg of the switch statement, but it's kind of a shrug for me.

Michael

quoted

quoted

 				 */
-				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;
 				}
--
2.25.1

Other than the seemingly spurious comment line,

Reviewed-by: Michael Kelley <redacted>

I wanted to make sure that we're on the same page: I could either expand
or just remove that comment line; no strong opinion.  Please let me know
what is your/reviewers' preference.

Thanks,
  Andrea