Re: [PATCH v3 33/40] scsi: storvsc: use 16-bits defined sense codes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Uninitialized stack variable `sense_hdr` is read in `storvsc_handle_error()` if a command completes with `scmnd->result == 0` but `vm_srb->srb_status` triggers an error.
--

commit d3f92ba91ef108f8874548b0b418a3d64212ceaf
Author: Damien Le Moal <dlemoal@kernel.org>

scsi: storvsc: use 16-bits defined sense codes

This commit refactors the storvsc driver to use the 16-bit sense_code field
from struct scsi_sense_hdr instead of hardcoded additional sense codes. This
improves code clarity by using defined enum values from scsi_sense.h.

> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index ef3cd3bc7390e..625db97d1a03e 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c

[ ... ]

> @@ -1141,8 +1145,7 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request,
>  	}
>  
>  	if (vm_srb->srb_status != SRB_STATUS_SUCCESS) {
> -		storvsc_handle_error(vm_srb, scmnd, host, sense_hdr.asc,
> -					 sense_hdr.ascq);
> +		storvsc_handle_error(vm_srb, scmnd, host, &sense_hdr);

[Severity: High]
This isn't a bug introduced by this patch, but does this pass an
uninitialized stack variable to the error handler?

Looking earlier in storvsc_command_completion() in
drivers/scsi/storvsc_drv.c, the sense_hdr stack variable is only populated
if scmnd->result is non-zero:

        struct scsi_sense_hdr sense_hdr;
        ...
        if (scmnd->result) {
                sense_ok = scsi_normalize_sense(scmnd->sense_buffer,
                                SCSI_SENSE_BUFFERSIZE, &sense_hdr);
        ...

If an error occurs where vm_srb->srb_status triggers an error
(e.g., SRB_STATUS_INTERNAL_ERROR) with SRB_STATUS_AUTOSENSE_VALID set,
but scsi_status is 0, then scmnd->result will also be 0.

Because scsi_normalize_sense() is bypassed, sense_hdr remains completely
uninitialized. storvsc_handle_error() will then evaluate this uninitialized
memory:

storvsc_handle_error()
    if (sshdr->sense_code == CAPACITY_DATA_HAS_CHANGED) {
        ...

If the garbage data doesn't randomly match the checked sense codes, the
function returns without setting DID_ERROR. 

Can this result in the SCSI midlayer receiving a supposedly successful command
with 0 bytes transferred, resulting in an infinite IO retry loop for that
request?

>  		/*
>  		 * The Windows driver set data_transfer_length on
>  		 * SRB_STATUS_DATA_OVERRUN. On other errors, this value

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904002644.157668-1-dlemoal@kernel.org?part=33