Re: [PATCH v8 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 2 potential issue(s) to consider:
- [High] Flawed LUN validation logic in atapi_xlat() incorrectly checks the host's max_lun instead of just the device limit, triggering a WARN_ON_ONCE and failing commands for ATAPI devices attached to SAS HBAs.
- [High] Unconditionally clearing bits 7:5 of CDB byte 1 in `atapi_xlat()` corrupts commands that repurpose these bits, such as SECURITY PROTOCOL IN/OUT.
--

--- Patch [3]: [PATCH v8 3/6] ata: libata-scsi: route non-zero LUN commands for multi-LUN ATAPI ---
commit 3d48864344b5c5d234865e96735b1a7de55c13b5
Author: Phil Pemberton <philpem@philpem.me.uk>

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

This commit modifies libata-scsi to support routing commands to non-zero
ATAPI LUNs. It updates __ata_scsi_find_dev to allow non-zero LUNs for
ATAPI devices and modifies atapi_xlat to encode the LUN into the SCSI-2
CDB for non-zero LUNs.

> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3012,6 +3012,20 @@ 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).
> +	 * Always clear those bits; only set them for non-zero LUNs.
> +	 */
> +	qc->cdb[1] = qc->cdb[1] & 0x1f;

[Severity: High]
Does unconditionally clearing bits 7:5 of CDB byte 1 corrupt commands that
repurpose these bits?

For example, in SECURITY PROTOCOL IN (0xA2) and OUT (0xB5) commands, byte 1
is used as the 8-bit Security Protocol field. Stripping the top 3 bits would
mutate protocol 0xEF into 0x0F, which could break features like Trusted
Computing (e.g., AACS on Blu-Ray, Opal) if a userspace application issues 
raw SG_IO commands to the ATAPI device.

> +	if (unlikely(scmd->device->lun)) {
> +		if (WARN_ON_ONCE(scmd->device->host->max_lun > ATAPI_MAX_LUN ||

[Severity: High]
Will this WARN_ON_ONCE incorrectly trigger and fail commands for ATAPI 
devices attached to SAS HBAs?

If a multi-LUN ATAPI device is attached to a host controller that advertises
a large max_lun (such as libsas drivers like hisi_sas which set
shost->max_lun = ~0), the condition checking if the host's max_lun is
greater than ATAPI_MAX_LUN evaluates to true.

This would unconditionally fail commands directed to non-zero LUNs with
DID_ERROR, breaking multi-LUN ATAPI support on modern HBAs.

> +				 scmd->device->lun >= scmd->device->host->max_lun)) {
> +			scmd->result = DID_ERROR << 16;
> +			return 1;
> +		}
> +		qc->cdb[1] |= (u8)scmd->device->lun << 5;
> +	}
> +
>  	qc->complete_fn = atapi_qc_complete;

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