From: Shao Mingyin ATA PASS-THROUGH(32) has no MULTIPLE_COUNT field. Byte 1 is CONTROL, including vendor-specific bits 7:6, but libata interprets bits 7:5 as MULTIPLE_COUNT. This rejects non-multiple commands with those bits set and can print spurious count mismatch warnings for multiple commands. The reserved bits 7:5 of byte 10 are not checked. Reject nonzero reserved bits in byte 10 with field pointer 10. Skip the MULTIPLE_COUNT checks for the 32-byte form while retaining the device multiple-mode check for READ/WRITE MULTIPLE commands. Also point to byte 10 for invalid PROTOCOL. Keep the 12- and 16-byte count checks unchanged. Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.") Suggested-by: Niklas Cassel Signed-off-by: Shao Mingyin --- Changes in v4 (Niklas Cassel and Damien Le Moal): - Check byte 10 bits 7:5 and report fp=10. CONTROL byte 1 is not a MULTIPLE_COUNT field and its bits 7:6 are vendor specific. - Skip the short-CDB MULTIPLE_COUNT check for ATA-32, retain support for READ/WRITE MULTIPLE, and correct the misleading comment. - Split ata_dev_warn() after dev, as requested by Damien. - Do not carry the v3 Reviewed-by tag across this behavioral change. Changes in v3 (Damien Le Moal): - Added a byte-1 bits 7:5 check with fp=1. This checked CONTROL instead of the reserved bits and is corrected in v4. - Retained the short-CDB count and device multiple-mode checks. - Updated the subject, description and CONTROL-byte test expectations. 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. v3: https://lore.kernel.org/all/20261112220120113cXLhRk6iDVvhfgGPoZ5vJ@zte.com.cn/ v2: https://lore.kernel.org/all/20261111223129430zClEJF8TDgu_h8ZYM843Q@zte.com.cn/ v1: https://lore.kernel.org/all/20261104220939917jqg8_alGN5sACGWq5M9D-@zte.com.cn/ Review: https://lore.kernel.org/all/avcujpOaXJPfjgxQ@ryzen/ Validation on x86-64 with GCC 13 and UBSAN: a userspace harness compiles the translation and completion functions verbatim with SCSI/device stubs. It crosses all 256 CONTROL values and all eight byte-10 high-bit patterns for non-multiple and READ/WRITE MULTIPLE commands, checking rejection with field pointer 10 and no CONTROL-based count warnings. It also checks the device multiple-mode and 12-/16-byte count checks, service actions, CK_COND and field pointers. Sense construction and device access use stubs; this is not a kernel runtime test. drivers/ata/libata-scsi.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index a61fb03312793ca94d81d8061a16c66e786c4cf3..8d321197803a839d9586e9f1436d1fbe3f5383c6 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,34 @@ 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)) { + /* Sanity check for pio multi commands. */ + if (cdb[0] == VARIABLE_LENGTH_CMD && (cdb[10] & 0xe0)) { + fp = 10; + goto invalid_fld; + } + + if (cdb[0] != VARIABLE_LENGTH_CMD && + (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); + } } /* -- 2.27.0