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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] LUN validation incorrectly checks the host's max_lun instead of the device's LUN limit, breaking SAS HBAs and introducing a kernel panic vector.
- [High] Unconditionally clearing bits 7:5 of CDB byte 1 destroys command-specific parameters for standard MMC commands, breaking Blu-ray decryption.
- [High] Error handling routines for ATAPI do not propagate the non-zero LUN, causing atapi_eh_request_sense() to incorrectly fetch sense data for LUN 0.
--

commit 0fa4117328a093e17803a117416e6ddc64dee46b
Author: Phil Pemberton <philpem@philpem.me.uk>

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

This patch modifies __ata_scsi_find_dev() to allow non-zero LUNs for ATAPI
devices and updates atapi_xlat() to encode the LUN into CDB byte 1 as
expected by older SCSI-2 era devices.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index b65358955cf17..2e3b5fd41d05f 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> @@ -2958,6 +2958,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]
Could this unconditional masking cause a regression by destroying
command-specific parameters for standard MMC commands?

Modern ATAPI/MMC devices reuse these bits for essential parameters since they
no longer rely on LUN addressing inside the CDB. For example, the REPORT KEY
command (0xA4) relies on bits 7:6 of byte 1 for the Key Class. Clearing
these bits forces the Key Class to 0 (DVD CSS), which breaks Blu-ray AACS
decryption tools for SATA/ATAPI optical drives.

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

[Severity: High]
Does this check incorrectly validate against the host max_lun instead of the
device LUN limit?

On SAS HBAs (like mpt3sas), host->max_lun is legitimately set to very large
values (e.g., 16895). Because 16895 > 8 (ATAPI_MAX_LUN), any command sent to
a non-zero LUN on an ATAPI device attached to such an HBA will trigger this
WARN_ON_ONCE and fail with DID_ERROR. 

On systems with panic_on_warn enabled, this will cause a kernel panic when a
multi-LUN ATAPI device is scanned.

[Severity: High]
Is there a missing update for error handling routines to propagate the
non-zero LUN, leading to a regression in sense data retrieval?

While this patch routes non-zero LUN commands by mutating CDB byte 1 in
atapi_xlat(), it appears this routing is missing from error handling. When a
command fails, ata_eh_analyze_tf() calls atapi_eh_request_sense().

In drivers/ata/libata-eh.c:atapi_eh_request_sense():
    u8 cdb[ATAPI_CDB_LEN] =
        { REQUEST_SENSE, 0, 0, 0, SCSI_SENSE_BUFFERSIZE, 0 };

This constructs a hardcoded REQUEST_SENSE CDB with byte 1 explicitly set to
0. If an I/O error occurs on LUN 1, the kernel will fetch the sense data
for LUN 0 instead. 

This could cause the SCSI midlayer to receive incorrect error status,
potentially masking legitimate I/O errors and leading to infinite retry
loops.

> +			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/20260611024356.2769320-1-philpem@philpem.me.uk?part=3