Message ID | 20211005114103.3411-1-parri.andrea@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | scsi: storvsc: Fix validation for unsolicited incoming packets | expand |
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Tuesday, October 5, 2021 4:41 AM > > 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://lore.kernel.org/all/20210928163732.5908-1-parri.andrea@gmail.com/T/#u > > 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 > + 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. > 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 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
> > @@ -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 > > > > @@ -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. > > > >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. :-) > > 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
> > > 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. > > > > > > > >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. :-) My (current) best guess is here: https://lkml.kernel.org/r/20211006132026.4089-1-parri.andrea@gmail.com Thanks, Andrea
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; }