scsi: storvsc: Set correct data length for sending SCSI command without payload

From: Long Li <longli@microsoft.com>

In StorVSC, payload->range.len is used to indicate if this SCSI command
carries payload. This data is allocated as part of the private driver
data by the upper layer and may get passed to lower driver uninitialized.

If a SCSI command doesn't carry payload, the driver may use this value as
is for communicating with host, resulting in possible corruption.

Fix this by always initializing this value.

Fixes: be0cf6ca301c ("scsi: storvsc: Set the tablesize based on the information given by the host")
Cc: stable@kernel.org
Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/scsi/storvsc_drv.c | 1 +
 1 file changed, 1 insertion(+)

On 1/16/2025 3:59 PM, longli@linuxonhyperv.com wrote:
> From: Long Li <longli@microsoft.com>
> 
> In StorVSC, payload->range.len is used to indicate if this SCSI command
> carries payload. This data is allocated as part of the private driver
> data by the upper layer and may get passed to lower driver uninitialized.
> 
> If a SCSI command doesn't carry payload, the driver may use this value as
> is for communicating with host, resulting in possible corruption.
> 
> Fix this by always initializing this value.

Awesome that you've caught that elusive critter, thank you! :)

Tested-by: Roman Kisel <romank@linux.microsoft.com>
Reviewed-by: Roman Kisel <romank@linux.microsoft.com>

> 
> Fixes: be0cf6ca301c ("scsi: storvsc: Set the tablesize based on the information given by the host")
> Cc: stable@kernel.org
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>   drivers/scsi/storvsc_drv.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 7ceb982040a5..ca5e5c0aeabf 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1789,6 +1789,7 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd)
>   
>   	length = scsi_bufflen(scmnd);
>   	payload = (struct vmbus_packet_mpb_array *)&cmd_request->mpb;
> +	payload->range.len = 0;
>   	payload_sz = 0;
>   
>   	if (scsi_sg_count(scmnd)) {

From: longli@linuxonhyperv.com <longli@linuxonhyperv.com>Sent: Thursday, January 16, 2025 4:00 PM
> 
> In StorVSC, payload->range.len is used to indicate if this SCSI command
> carries payload. This data is allocated as part of the private driver
> data by the upper layer and may get passed to lower driver uninitialized.

I had always thought the private driver data *is* initialized to zero by the
upper layer. Indeed, scsi_queue_rq() calls scsi_prepare_cmd(), which
zeros the private driver data as long as the driver does not specify a
custom function to do the initialization (and storvsc does not).  So
I'm curious -- what's the execution path where this initialization doesn't
happen?

Michael

> 
> If a SCSI command doesn't carry payload, the driver may use this value as
> is for communicating with host, resulting in possible corruption.
> 
> Fix this by always initializing this value.
> 
> Fixes: be0cf6ca301c ("scsi: storvsc: Set the tablesize based on the information given by
> the host")
> Cc: stable@kernel.org
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  drivers/scsi/storvsc_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 7ceb982040a5..ca5e5c0aeabf 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1789,6 +1789,7 @@ static int storvsc_queuecommand(struct Scsi_Host *host,
> struct scsi_cmnd *scmnd)
> 
>  	length = scsi_bufflen(scmnd);
>  	payload = (struct vmbus_packet_mpb_array *)&cmd_request->mpb;
> +	payload->range.len = 0;
>  	payload_sz = 0;
> 
>  	if (scsi_sg_count(scmnd)) {
> --
> 2.43.0
>