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

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

[longli]

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(+)

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

Re: [PATCH] scsi: storvsc: Set correct data length for sending SCSI command without payload

[Roman Kisel]

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)) {

-- 
Thank you,
Roman

RE: [PATCH] scsi: storvsc: Set correct data length for sending SCSI command without payload

[Michael Kelley]

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
> 

RE: [PATCH] scsi: storvsc: Set correct data length for sending SCSI command without payload

[Long Li]

> > 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

SCSI mid layer may send commands to lower driver without initializing private data. 
For example, scsi_send_eh_cmnd() may send TEST_UNIT_READY and REQUEST_SENSE to lower layer driver without initializing private data.

I don't know if there are other places doing similar things outside scsi_error.c, but storvsc is already calling memset() on its private data:
(in storvsc_queuecommand)
memset(&cmd_request->vstor_packet, 0, sizeof(struct vstor_packet));

The assumption is that private data is not guaranteed to be 0.

Long

RE: [PATCH] scsi: storvsc: Set correct data length for sending SCSI command without payload

[Michael Kelley]

From: Long Li <longli@microsoft.com> Sent: Monday, January 20, 2025 3:21 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
> 
> SCSI mid layer may send commands to lower driver without initializing private data.
> For example, scsi_send_eh_cmnd() may send TEST_UNIT_READY and REQUEST_SENSE
> to lower layer driver without initializing private data.

Right. Thanks for pointing out this path that I wasn't aware of. My
suggestion would be to add a little more detail in the commit message,
including identifying this path where the private data isn't zero'ed. Some
future developer will wonder what's going on and appreciate having
the specific reason provided.

> 
> I don't know if there are other places doing similar things outside scsi_error.c, but
> storvsc is already calling memset() on its private data:
> (in storvsc_queuecommand)
> memset(&cmd_request->vstor_packet, 0, sizeof(struct vstor_packet));
> 
> The assumption is that private data is not guaranteed to be 0.
> 

That memset() was added relatively recently (in 2020) when doing the driver
hardening for Confidential VMs. At the time, I was thinking it was needed
because the private data isn't zero'ed, but later discovered what
scsi_prepare_cmd() does. Then I was thinking the memset() is duplicative
and wasteful, but didn't ever go back and remove it.

It seems like the SCSI subsystem has a generic inconsistency here in that
scsi_prepare_cmd() *does* zero the private data. In an attempt to give the
low level driver a clean slate, that zero'ing is done when a command is first
assigned to a request. But in the error case, the command can be re-used,
or "hijacked" per the comment for scsi_send_eh_cmnd(), and the private
data does not get zero'ed again. If the low level driver isn't guaranteed a
clean slate, then the zero'ing in scsi_prepare_cmd() is arguably not needed.

But that generic inconsistency is a different problem for another day. I'm
good with your fix in storvsc.

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

RE: [PATCH] scsi: storvsc: Set correct data length for sending SCSI command without payload

[Long Li]

> > SCSI mid layer may send commands to lower driver without initializing private
> data.
> > For example, scsi_send_eh_cmnd() may send TEST_UNIT_READY and
> > REQUEST_SENSE to lower layer driver without initializing private data.
> 
> Right. Thanks for pointing out this path that I wasn't aware of. My suggestion
> would be to add a little more detail in the commit message, including identifying
> this path where the private data isn't zero'ed. Some future developer will wonder
> what's going on and appreciate having the specific reason provided.

I sent v2 adding more details to the comment.

Long

RE: [PATCH] scsi: storvsc: Set correct data length for sending SCSI command without payload

[Michael Kelley]

From: Long Li <longli@microsoft.com>
> 
> > > SCSI mid layer may send commands to lower driver without initializing private
> > data.
> > > For example, scsi_send_eh_cmnd() may send TEST_UNIT_READY and
> > > REQUEST_SENSE to lower layer driver without initializing private data.
> >
> > Right. Thanks for pointing out this path that I wasn't aware of. My suggestion
> > would be to add a little more detail in the commit message, including identifying
> > this path where the private data isn't zero'ed. Some future developer will wonder
> > what's going on and appreciate having the specific reason provided.
> 
> I sent v2 adding more details to the comment.
> 

Thanks.  Looks good to me.

Michael