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

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@xxxxxxxxx>
> ---
> 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@xxxxxxxxx/T/#u 
> [2] https://lore.kernel.org/all/20210928163732.5908-1-parri.andrea@xxxxxxxxx/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@xxxxxxxxxxxxx>