Re: [PATCH v6 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 3 potential issue(s) to consider:
- [High] Returning AC_ERR_INVALID without setting scmd->result causes the SCSI layer to treat the aborted command as a success.
- [High] Unconditional modification of CDB byte 1 clears parameter bits for valid MMC and SCSI commands, corrupting them.
- [Medium] The __ata_scsi_find_dev() function accepts non-zero LUNs for all ATAPI devices, leading to WARN_ON_ONCE triggers and phantom devices during manual SCSI scans.
--

commit 23cd1ea8143d086cbe9cf8a4825c18b4ede2d943
Author: Phil Pemberton <philpem@philpem.me.uk>

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

This patch updates libata-scsi to route commands to non-zero LUNs for ATAPI
devices. It modifies __ata_scsi_find_dev() to allow non-zero LUNs for ATAPI
devices and updates atapi_xlat() to encode the LUN into bits 7:5 of CDB byte 1.

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

[Severity: High]
Does returning AC_ERR_INVALID here without setting scmd->result cause the
SCSI layer to treat the aborted command as a success?

The caller, ata_scsi_translate(), drops the command and calls scsi_done(cmd)
when xlat_func returns an error, but it doesn't set cmd->result to an error
status:

drivers/ata/libata-scsi.c:ata_scsi_translate() {
    ...
done:
    ata_qc_free(qc);
    scsi_done(cmd);
    return 0;
}

Since scmd->result defaults to SAM_STAT_GOOD (0), the SCSI midlayer will
believe the command succeeded.

This can cause the INQUIRY command to succeed, resulting in phantom LUNs.
For write operations, it can cause silent data corruption as the data is
dropped but reported as successfully written.

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

[Severity: High]
Does this unconditionally overwrite CDB byte 1 and corrupt parameter bits for
valid MMC and SCSI commands?

Some modern SCSI commands use bits 7:5 of byte 1 for actual parameters, such
as RDPROTECT in READ (16) and WRITE (16), or MULTIPLE_COUNT in ATA
PASS-THROUGH (16).

If this overwrites those bits, the parameters will be silently corrupted. Is
it possible to rely on sdev->lun_in_cdb which the SCSI midlayer uses to
handle LUN injection safely for legacy devices?

[ ... ]

> @@ -3063,6 +3072,29 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
>  {
>  	int devno;
>  
> +	/*
> +	 * Non-zero LUN is only legal for ATAPI devices, since they can
> +	 * legitimately expose more than one LUN (PD/CD combos, CD changers).
> +	 * Handle that case up front so the LUN-0 path below stays unchanged.
> +	 */
> +	if (unlikely(scsidev->lun)) {
> +		struct ata_device *dev;
> +
> +		if (!sata_pmp_attached(ap)) {
> +			if (unlikely(scsidev->channel))
> +				return NULL;
> +			devno = scsidev->id;
> +		} else {
> +			if (unlikely(scsidev->id))
> +				return NULL;
> +			devno = scsidev->channel;
> +		}
> +		dev = ata_find_dev(ap, devno);
> +		if (!dev || dev->class != ATA_DEV_ATAPI)
> +			return NULL;
> +		return dev;

[Severity: Medium]
Does permitting non-zero LUNs for all ATAPI devices here allow users to
trigger the WARN_ON_ONCE() in atapi_xlat()?

Since dev->nr_luns remains 1 for standard single-LUN ATAPI devices that do
not have the BLIST_FORCELUN quirk, a manual SCSI scan (e.g., via sysfs) on a
non-zero LUN will reach atapi_xlat() and hit the WARN_ON_ONCE().

> +	}
> +

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