Re: [PATCH v3 1/4] ata: libata-scsi: reject reserved bits in 32-byte passthru

[Niklas Cassel <cassel@kernel.org>]

On Thu, Nov 12, 2026 at 10:01:20PM +0800, shao.mingyin@zte.com.cn wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> ATA PASS-THROUGH(32) has no MULTIPLE_COUNT field. Byte 1 is CONTROL,
> and bits 7:5 are reserved or obsolete. Interpreting these bits as a
> multiple count can issue spurious count mismatch warnings for multiple
> commands instead of rejecting the invalid CDB.
> 
> Reject nonzero byte 1 bits 7:5 with field pointer 1. Skip the CDB count
> comparison for the 32-byte form while retaining the device multiple-mode
> check. Also point to byte 10 for invalid PROTOCOL. The 12- and 16-byte
> MULTIPLE_COUNT checks remain unchanged.
> 
> Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> ---
> Changes in v3 (Damien Le Moal):
> - Reject nonzero reserved/obsolete bits 7:5 of byte 1 with fp=1.
> - Retain the short-CDB count and device multiple-mode checks.
> - Update the subject, description and CONTROL-byte test expectations.
> v2: https://lore.kernel.org/all/20261111223129430zClEJF8TDgu_h8ZYM843Q@zte.com.cn/
> 
> Validation on x86-64 with GCC 13 and UBSAN: a userspace harness compiles
> ata_scsi_pass_thru() verbatim with SCSI/device stubs. All 256 CONTROL
> values are checked for both non-multiple and multiple commands. Nonzero
> bits 7:5 are rejected with field pointer 1; values with those bits clear
> are accepted without count warnings. The device multiple-mode check and
> 12-/16-byte count checks still pass. Field pointers are observed helper
> arguments; sense construction and device access use stubs.
> 
>  drivers/ata/libata-scsi.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a61fb03312793ca94d81d8061a16c66e786c4cf3..0bc147cf4c0fa50c7d5b84029b7fb1a548c6f1a3 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3388,7 +3388,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>  
>  	tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset]);
>  	if (tf->protocol == ATA_PROT_UNKNOWN) {
> -		fp = 1;
> +		fp = 1 + cdb_offset;
>  		goto invalid_fld;
>  	}
>  
> @@ -3573,27 +3573,32 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>  		goto invalid_fld;
>  	}
>  
> -	/* sanity check for pio multi commands */
> +	/* Sanity check for pio multi commands (not supported with ATA 32). */

This comment is misleading.

READ/WRITE MULTIPLE commands are still accepted with the 32-byte CDB, it is
only the count comparison that will be skipped.

libata reads dev->multi_count from IDENTIFY word 59, during probe/revalidate,
and after receiving a ATA_CMD_SET_MULTI command.

The MULTI_COUNT field is obsolete for 12-byte and 16-byte CDBs in SAT-6
(tables 203 and 209), and the 32-byte CDB has no such field (table 210).

The MULTI_COUNT field was only compared with dev->multi_count, and
if there was a mismatch, libata simply printed a warning:
"invalid multi_count %u ignored", yet the command was still sent and
limited by dev->multi_count (see e.g. ata_pio_sectors() for SFF
controllers).


> +	if (cdb[0] == VARIABLE_LENGTH_CMD && (cdb[1] & 0xe0)) {
> +		fp = 1;
> +		goto invalid_fld;
> +	}

In the 32-byte CDB, byte 1 is CONTROL (SAT-6 r2 spec, table 210), and its
bits 7:6 are vendor specific (6.5, table 10), not reserved or obsolete as
the commit message says.

The reserved bits in place of MULTIPLE_COUNT are bits 7:5 of byte 10.

So I think the test should be:

if (cdb[0] == VARIABLE_LENGTH_CMD && (cdb[10] & 0xe0)) {
	fp = 10;
	goto invalid_fld;
}


Kind regards,
Niklas