Re: [PATCH v5 3/6] ata: libata-scsi: route non-zero LUN commands for multi-LUN ATAPI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Unconditional LUN injection into CDB byte 1 clobbers command-specific flags for modern ATAPI commands.
- [Critical] Returning AC_ERR_INVALID from atapi_xlat without setting cmd->result causes silent command success (Silent Data Corruption).
- [High] Reachable WARN_ON_ONCE splat and broken LUN > 0 feature due to hardcoded dev->nr_luns.
- [High] Lifecycle functions fail to clean up LUNs > 0 when dev->nr_luns remains 1, leading to resource leaks and use-after-free.
--

commit 3063d8fc6690940538378993230bab65cc937027
Author: Phil Pemberton <philpem@philpem.me.uk>

ata: libata-scsi: route non-zero LUN commands for multi-LUN ATAPI

This commit updates __ata_scsi_find_dev() to allow commands directed at
ATAPI devices with non-zero LUNs to proceed. It also modifies atapi_xlat()
to encode the LUN into bits 7:5 of CDB byte 1 to support legacy SCSI-2
era ATAPI devices.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7c3d31dc49a17..2d714efc855f0 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2953,6 +2953,15 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
>  	memset(qc->cdb, 0, dev->cdb_len);
>  	memcpy(qc->cdb, scmd->cmnd, scmd->cmd_len);
>  
> +	/*
> +	 * SCSI-2 CDB LUN encoding: bits 7:5 of byte 1 (3-bit field).
> +	 * The SCSI layer caps the LUN at shost->max_lun (<= ATAPI_MAX_LUN),
> +	 * so this should never trip; warn and reject if it does.
> +	 */
> +	if (WARN_ON_ONCE(scmd->device->lun >= dev->nr_luns))
> +		return AC_ERR_INVALID;

In the libata subsystem, dev->nr_luns is statically initialized to 1.
If an administrator triggers a manual SCSI scan to LUN 1 via sysfs, will
this check evaluate to true and trigger a kernel warning splat?

Does returning AC_ERR_INVALID here also block the multi-LUN feature for
standard ATAPI devices that do not have a specific quirk to increment
dev->nr_luns?

If atapi_xlat() returns AC_ERR_INVALID here, ata_scsi_translate() will
detect the non-zero return and directly call scsi_done().

Because cmd->result remains at its initialized value of 0 (SAM_STAT_GOOD),
will the SCSI and block layers treat the dropped I/O as successfully
completed? Could this return uninitialized memory to the caller for
read commands?

Additionally, if a non-zero LUN is instantiated because a manual sysfs
scan silently succeeds due to the AC_ERR_INVALID behavior above, but
dev->nr_luns remains 1, will lifecycle loops in functions like
ata_scsi_remove_dev() fail to process the extra LUNs?

Could this leak the scsi_device structure for LUN 1 and cause a
use-after-free when the underlying ATA port is destroyed?

> +	qc->cdb[1] = (qc->cdb[1] & 0x1f) | ((u8)scmd->device->lun << 5);

For modern ATAPI devices using the SCSI-3/MMC command set, bits 7:5 of
CDB byte 1 are repurposed for command-specific parameters, such as the
RDPROTECT and WRPROTECT flags in read/write(10), or the blanking type
bits in the blank command.

Does unconditionally masking and setting these bits silently corrupt
those valid flags?

Also, since the Linux SCSI midlayer automatically performs this injection
based on the sdev->lun_in_cdb flag for legacy SCSI-2 devices, is this
manual injection redundant?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512202728.299414-1-philpem@philpem.me.uk?part=3