Thread (5 messages) 5 messages, 2 authors, 2021-10-06

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

From: Andrea Parri <parri.andrea@gmail.com>
Date: 2021-10-06 13:39:39
Also in: linux-scsi, lkml

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help