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

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

[Andrea Parri (Microsoft)]

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>
---
Changes since v1[1]:
  - Use sizeof(enum vstor_packet_operation) instead of 48 (Michael Kelley)
  - Filter out FCHBA_DATA packets (Michael Kelley)

Changes since RFC[2]:
  - Merge length checks (Haiyang Zhang)

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

 drivers/scsi/storvsc_drv.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index ebbbc1299c625..4869ebad7ec97 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1285,11 +1285,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 : sizeof(enum vstor_packet_operation);
 
-		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;
 		}
 
@@ -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.
+				 *
+				 * XXX.  Filter out all "invalid" operations.
 				 */
-				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

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

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, October 6, 2021 6:20 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>
> ---
> Changes since v1[1]:
>   - Use sizeof(enum vstor_packet_operation) instead of 48 (Michael Kelley)
>   - Filter out FCHBA_DATA packets (Michael Kelley)
> 
> Changes since RFC[2]:
>   - Merge length checks (Haiyang Zhang)
> 
> [1] https://lore.kernel.org/all/20211005114103.3411-1-parri.andrea@gmail.com/T/#u 
> [2] https://lore.kernel.org/all/20210928163732.5908-1-parri.andrea@gmail.com/T/#u 
> 
>  drivers/scsi/storvsc_drv.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index ebbbc1299c625..4869ebad7ec97 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1285,11 +1285,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 : sizeof(enum vstor_packet_operation);
> 
> -		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;
>  		}
> 
> @@ -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".

>  				 */
> -				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@microsoft.com>

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

[Andrea Parri]

> > @@ -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@microsoft.com>

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

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

[Michael Kelley]

From: Andrea Parri <parri.andrea@gmail.com> Sent: Wednesday, October 6, 2021 9:18 AM
> > > @@ -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.
> 

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

> 
> > >  				 */
> > > -				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@microsoft.com>
> 
> 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