Re: [PATCH v3 09/40] scsi: sr: 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 `sshdr` is read if `scsi_test_unit_ready()` fails early with a negative error code.
--

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

scsi: sr: use 16-bits defined sense codes
    
Refactor the SCSI CDROM driver to use the 16-bits sense_code field of
struct scsi_sense_hdr and replace all hard-coded additional sense codes
and additional sense code qualifiers with the enum values defined in
include/scsi/scsi_sense.h.

> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index c36c54ecd354b..4f2e8d1be6da8 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -253,7 +253,8 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
>  	 * (ASC 0x3a).
>  	 */
>  	cd->media_present = scsi_status_is_good(ret) ||
> -		(scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
> +		(scsi_sense_valid(&sshdr) &&
> +		 scsi_sense_asc(&sshdr) != ASC_MEDIUM_NOT_PRESENT);

[Severity: High]
This is a pre-existing issue, but since the patch modifies this expression, I
wanted to ask about the uninitialized use of the sshdr variable here. 

In sr_check_events(), sshdr is declared on the stack without being zeroed. If
scsi_test_unit_ready() fails early and returns a negative error code (like
-ENOMEM or -ENODEV), sshdr is never populated. When ret is negative,
scsi_status_is_good(ret) evaluates to false, which causes scsi_sense_valid()
to evaluate the uninitialized sshdr structure.

Should ret be checked to ensure it is non-negative before processing the sense
data?

>  
>  	if (last_present != cd->media_present)
>  		cd->device->changed = 1;

[ ... ]

> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index 089653018d32c..e4ec0f63cdccb 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
[ ... ]
> @@ -318,18 +314,18 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)
>  	}
>  
>  	/*
>  	 * Using CD_TRAY_OPEN results in an START_STOP_UNIT to close
>  	 * the tray, which resolves the initialization requirement.
>  	 */
> -	if (scsi_sense_valid(&sshdr) && sshdr.sense_key == NOT_READY
> -			&& sshdr.asc == 0x04 && sshdr.ascq == 0x02)
> +	if (scsi_sense_valid(&sshdr) && sshdr.sense_key == NOT_READY &&
> +	    sshdr.sense_code == LU_NOT_READY_INITIALIZING_COMMAND_REQUIRED)
>  		return CDS_TRAY_OPEN;
>  
>  	/*
>  	 * 0x04 is format in progress .. but there must be a disc present!
>  	 */
> -	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04)
> +	if (sshdr.sense_key == NOT_READY &&
> +	    scsi_sense_asc(&sshdr) == ASC_LU_NOT_READY)

[Severity: High]
This is also a pre-existing issue, but I wanted to mention it since the code
is being updated.

Similar to the issue in sr_check_events(), sshdr is uninitialized in
sr_drive_status() if scsi_test_unit_ready() returns a negative error code.
Here, it looks like sshdr.sense_key is accessed unconditionally without even
being guarded by a call to scsi_sense_valid().

Could this lead to incorrect drive status reporting if the uninitialized
stack memory happens to match NOT_READY?

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