Re: [PATCH v2 1/4] ata: libata-scsi: skip MULTIPLE_COUNT for 32-byte passthru

[Damien Le Moal <dlemoal@kernel.org>]

On 11/11/26 23:31, shao.mingyin@zte.com.cn wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> ATA PASS-THROUGH(32) has no MULTIPLE_COUNT field. Reading byte 1 as
> that field instead interprets the variable-length CDB CONTROL byte,
> which can reject a non-multiple command or cause a spurious count
> mismatch warning for READ/WRITE MULTIPLE.
> 
> Skip the CDB count checks for the 32-byte form while retaining the
> check that multiple mode is enabled on the device. Also report byte 10
> for an invalid PROTOCOL, where that field resides in a 32-byte CDB.
> 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 v2 (Niklas Cassel):
> - Skip the nonexistent MULTIPLE_COUNT field for the 32-byte CDB;
>   preserve the device multiple-mode check and the short-CDB checks.
> - Move the invalid-PROTOCOL field pointer fix here from patch 3.
> - Regenerate with git format-patch; do not edit diff hunks or indexes.
> - Do not carry the v1 Reviewed-by tag over the reworked code.
> v1: https://lore.kernel.org/all/20261104220939917jqg8_alGN5sACGWq5M9D-@zte.com.cn/
> 
> Validation on x86-64 with GCC 13 and UBSAN: a userspace harness compiled
> ata_scsi_pass_thru() verbatim with SCSI/device stubs. An invalid PROTOCOL
> in a 32-byte CDB passed field pointer 1 to the invalid-field helper before
> the fix and 10 afterwards. CONTROL-byte variations no longer caused
> false rejection or multiple-count warnings. A device with multiple mode
> disabled still rejected READ/WRITE MULTIPLE; short-CDB checks were kept.
> 
>  drivers/ata/libata-scsi.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a61fb03312793ca94d81d8061a16c66e786c4cf3..227240514651ada8b5feb87a154833fb2cddf064 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,28 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>  		goto invalid_fld;
>  	}
>  
> -	/* sanity check for pio multi commands */
> -	if ((cdb[1] & 0xe0) && !is_multi_taskfile(tf)) {
> +	/* The 32-byte CDB has no MULTIPLE_COUNT field. */
> +	if (cdb[0] != VARIABLE_LENGTH_CMD &&
> +	    (cdb[1] & 0xe0) && !is_multi_taskfile(tf)) {

Sinc ATA 32 defines these beets as reserved/obsolete, we should check that they
are 0 and reject the command if they are not. So something like:

	/* Sanity check for pio multi commands (not supported with ATA 32). */
	if (cdb[0] == VARIABLE_LENGTH_CMD && (cdb[1] & 0xe0)) {
		fp = 1;
		goto invalid_fld;
	}

	if ((cdb[1] & 0xe0) && !is_multi_taskfile(tf)) {
		...

>  		fp = 1;
>  		goto invalid_fld;
>  	}
>  
>  	if (is_multi_taskfile(tf)) {
> -		unsigned int multi_count = 1 << (cdb[1] >> 5);
> -
>  		if (!dev->multi_count) {
>  			ata_scsi_set_sense(dev, scmd, ABORTED_COMMAND,
>  					   NO_ADDITIONAL_SENSE_INFORMATION);
>  			return 1;
>  		}
>  
> -		/* compare the passed through multi_count
> -		 * with the cached multi_count of libata
> -		 */
> -		if (multi_count != dev->multi_count)
> -			ata_dev_warn(dev, "invalid multi_count %u ignored\n",
> -				     multi_count);
> +		if (cdb[0] != VARIABLE_LENGTH_CMD) {
> +			unsigned int multi_count = 1 << (cdb[1] >> 5);
> +
> +			/* Compare with the cached multiple count of libata. */
> +			if (multi_count != dev->multi_count)
> +				ata_dev_warn(dev, "invalid multi_count %u ignored\n",
> +					     multi_count);
> +		}
>  	}
>  
>  	/*


-- 
Damien Le Moal
Western Digital Research