> > @@ -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.
>
> > + *
> > + * 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.
> > */
> > - 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 <mikelley@xxxxxxxxxxxxx>
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
