[PATCH v4 0/4] ata: libata-scsi: fix ATA PASS-THROUGH(32) handling

[PATCH v4 0/4] ata: libata-scsi: fix ATA PASS-THROUGH(32) handling

[shao.mingyin]

ATA PASS-THROUGH(32) still has translation and completion paths that
interpret fields as if the command used a 12- or 16-byte CDB.

Patch 1 checks the reserved bits in byte 10, skips the nonexistent
MULTIPLE_COUNT field, and corrects the invalid-PROTOCOL field pointer.
Patch 2 rejects unsupported service actions with sense data. Patch 3
honors CK_COND and generates SAT sense on completion. Patch 4 corrects
the remaining field pointers, including READ/WRITE LONG count errors.
The four patches can be applied independently.

Changes in v4:
- Patch 1: follow Niklas's correction to validate byte 10 bits 7:5
  with fp=10, leave CONTROL out of the count checks, and retain
  READ/WRITE MULTIPLE support. Correct the comment and description.
  Split the warning after dev, per Damien's nit. Extend the function
  tests to cross CONTROL values and byte-10 reserved bits.
- Patches 2-4: code and commit messages unchanged; retain Damien's
  Reviewed-by tags. Patch 1 needs review again after its behavior change.
Previous version: https://lore.kernel.org/all/20261112220113627e1oAXj5lbAzyI0Xe6Joud@zte.com.cn/

Codex assisted with the review changes, descriptions and validation
scripts. The updated code was function-tested and build-tested.

Shao Mingyin (4):
  ata: libata-scsi: reject reserved bits in 32-byte passthru
  ata: libata-scsi: reject unsupported ATA-32 service actions
  ata: libata-scsi: generate SAT sense data for ATA PASS-THROUGH(32)
  ata: libata-scsi: fix remaining passthru field pointers

 drivers/ata/libata-scsi.c | 70 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 20 deletions(-)

---
base-commit: ddfe51b0a0f5dc9e415cc2c8ad8db17532dd426b

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

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

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 <cassel@kernel.org>
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
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);
+		}
 	}
 
 	/*

[PATCH v4 2/4] ata: libata-scsi: reject unsupported ATA-32 service actions

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

ata_scsi_var_len_cdb_xlat() returns 1 for an unsupported service action
without setting sense data. ata_scsi_translate() then completes the
command with the SCSI result still zero, reporting SAM_STAT_GOOD even
though no ATA command was issued and no data was transferred.

Set ILLEGAL REQUEST/INVALID FIELD IN CDB before returning, with the
field pointer referring to the SERVICE ACTION field at byte 8.

Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
Changes in v4:
- Code and commit message unchanged; retain Damien Le Moal's Reviewed-by tag.
Previous version: https://lore.kernel.org/all/20261112220121378caZJpnCPmheSAmKX1Hl8g@zte.com.cn/

Changes in v3:
- Code unchanged; retain the Damien Le Moal Reviewed-by tag.
Previous version: https://lore.kernel.org/all/20261111223130666omUZ0JoWtfZjRFRrWPse8@zte.com.cn/

Changes in v2:
- No code changes; regenerate against the series base and keep the
  Reviewed-by tag.
v1: https://lore.kernel.org/all/20261104221102824qAKUWNkiSgZ1yRnQM8hI9@zte.com.cn/

Validation on x86-64 with GCC 13 and UBSAN: a userspace harness compiled
the translation functions verbatim with SCSI/device stubs. For service
action 0x1234, completion changed from result 0 to ILLEGAL REQUEST,
INVALID FIELD IN CDB; the invalid-field helper received field pointer 8.
A valid ATA_32 service action remained accepted.

 drivers/ata/libata-scsi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8d321197803a839d9586e9f1436d1fbe3f5383c6..e2847fb32e78b93fd0c8fd3b57e78ae3b17654b6 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -5034,7 +5034,13 @@ static unsigned int ata_scsi_var_len_cdb_xlat(struct ata_queued_cmd *qc)
 	if (sa == ATA_32)
 		return ata_scsi_pass_thru(qc);
 
-	/* unsupported service action */
+	/*
+	 * Unsupported service action.  Set the sense data explicitly, as
+	 * returning 1 without it makes ata_scsi_translate() complete the
+	 * command with the SCSI result still zero, that is, with
+	 * SAM_STAT_GOOD.
+	 */
+	ata_scsi_set_invalid_field(qc->dev, scmd, 8, 0xff);
 	return 1;
 }

[PATCH v4 3/4] ata: libata-scsi: generate SAT sense data for ATA PASS-THROUGH(32)

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

ata_scsi_qc_complete() only recognizes ATA PASS-THROUGH(12) and (16).
The 32-byte form takes the ordinary completion path, so CK_COND is
ignored and errors lack the ATA Status Return descriptor. Successful
commands with CK_COND set therefore complete GOOD instead of returning
CHECK CONDITION with the ATA register values.

Recognize ATA PASS-THROUGH(32) by its service action and read CK_COND
from byte 11 instead of byte 2, which belongs to the variable-length
CDB header.

Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
Cc: stable@vger.kernel.org
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
Changes in v4:
- Code and commit message unchanged; retain Damien Le Moal's Reviewed-by tag.
Previous version: https://lore.kernel.org/all/20261112220122679csrHZJU28_17KSmt_fom7@zte.com.cn/

Changes in v3:
- Code unchanged; retain the Damien Le Moal Reviewed-by tag.
Previous version: https://lore.kernel.org/all/202611112231319337i08AK7ZzIXm9DYFhmtXx@zte.com.cn/

Changes in v2 (Niklas Cassel):
- Move the unrelated invalid-PROTOCOL hunk to patch 1; this patch only
  changes the completion path.
- Regenerate with git format-patch so the index matches the diff.
- Keep the Reviewed-by tag; apart from the moved hunk this patch is
  unchanged from v1.
v1: https://lore.kernel.org/all/202611042211545962bhKeo8yjWPGFgya4g6LG@zte.com.cn/

Validation on x86-64 with GCC 13 and UBSAN: a userspace harness compiled
ata_scsi_qc_complete() verbatim with sense/completion stubs. For ATA_32,
CK_COND in byte 11 now selected pass-through sense and CHECK CONDITION;
CK_COND clear remained GOOD. Error and preexisting-sense paths selected
the pass-through helpers, and byte 2 did not act as CK_COND. The short
CDB paths and unsupported service-action control were also checked.

 drivers/ata/libata-scsi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e2847fb32e78b93fd0c8fd3b57e78ae3b17654b6..374918eae53ef4cb082bccd903773f06e6c55fa1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1888,9 +1888,12 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 	struct ata_link *link = qc->dev->link;
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 *cdb = cmd->cmnd;
+	u16 cdb_offset = cdb[0] == VARIABLE_LENGTH_CMD ? 9 : 0;
 	bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;
-	bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
-	bool is_ck_cond_request = cdb[2] & 0x20;
+	bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12 ||
+		(cdb[0] == VARIABLE_LENGTH_CMD &&
+		 get_unaligned_be16(&cdb[8]) == ATA_32);
+	bool is_ck_cond_request = cdb[2 + cdb_offset] & 0x20;
 	bool is_error = qc->err_mask != 0;
 
 	/* For ATA pass thru (SAT) commands, generate a sense block if

[PATCH v4 4/4] ata: libata-scsi: fix remaining passthru field pointers

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

Several error paths in ata_scsi_pass_thru() report field pointers for
the 12- or 16-byte CDB when rejecting a 32-byte CDB. Initiators then
receive INVALID FIELD IN CDB sense data pointing at an unrelated byte.
READ/WRITE LONG also reports PROTOCOL when its sector count is invalid.

Use the PROTOCOL offset and record the FEATURES, SECTOR COUNT and
COMMAND offsets for each CDB format. Report SECTOR COUNT separately
from PROTOCOL when validating READ/WRITE LONG.

Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
Suggested-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
Changes in v4:
- Code and commit message unchanged; retain Damien Le Moal's Reviewed-by tag.
Previous version: https://lore.kernel.org/all/20261112220124003NT-esXu4g2qMJ-uOMc-4G@zte.com.cn/

Changes in v3:
- Code unchanged; retain the Damien Le Moal Reviewed-by tag.
Previous version: https://lore.kernel.org/all/20261111223133188VjHq0FbmnzuEZWakS-EYD@zte.com.cn/

Changes in v2 (Niklas Cassel):
- Add a separate patch for the remaining PROTOCOL, FEATURES, COMMAND
  and READ/WRITE LONG sector-count field pointers.
v1: https://lore.kernel.org/all/20261104220832716LXKf7OaS9dt1VJ8Q959AG@zte.com.cn/

Validation on x86-64 with GCC 13 and UBSAN: a userspace harness compiled
the translation functions verbatim with SCSI/device stubs. The rejected
32-byte DMA/NCQ and READ/WRITE LONG protocols passed field pointer 10 to
the invalid-field helper, SET FEATURES passed 21, and TPM passed 25.
Invalid READ/WRITE LONG sector counts passed 4, 6 and 23 for 12-, 16-
and 32-byte CDBs. These are helper arguments observed in the harness.

 drivers/ata/libata-scsi.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 374918eae53ef4cb082bccd903773f06e6c55fa1..a5947bd591df8905e6b06f9851f75a46954f2c22 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3383,6 +3383,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	struct ata_device *dev = qc->dev;
 	const u8 *cdb = scmd->cmnd;
 	u16 fp;
+	u8 feature_offset, nsect_offset, command_offset;
 	u16 cdb_offset = 0;
 
 	/* 7Fh variable length cmd means a ata pass-thru(32) */
@@ -3443,6 +3444,9 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 		tf->lbah = cdb[12];
 		tf->device = cdb[13];
 		tf->command = cdb[14];
+		feature_offset = 4;
+		nsect_offset = 6;
+		command_offset = 14;
 		break;
 	case ATA_12:
 		/*
@@ -3457,6 +3461,9 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 		tf->lbah = cdb[7];
 		tf->device = cdb[8];
 		tf->command = cdb[9];
+		feature_offset = 3;
+		nsect_offset = 4;
+		command_offset = 9;
 		break;
 	default:
 		/*
@@ -3482,6 +3489,9 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 		tf->device = cdb[24];
 		tf->command = cdb[25];
 		tf->auxiliary = get_unaligned_be32(&cdb[28]);
+		feature_offset = 21;
+		nsect_offset = 23;
+		command_offset = 25;
 		break;
 	}
 
@@ -3499,8 +3509,12 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	case ATA_CMD_READ_LONG_ONCE:
 	case ATA_CMD_WRITE_LONG:
 	case ATA_CMD_WRITE_LONG_ONCE:
-		if (tf->protocol != ATA_PROT_PIO || tf->nsect != 1) {
-			fp = 1;
+		if (tf->protocol != ATA_PROT_PIO) {
+			fp = 1 + cdb_offset;
+			goto invalid_fld;
+		}
+		if (tf->nsect != 1) {
+			fp = nsect_offset;
 			goto invalid_fld;
 		}
 		qc->sect_size = scsi_bufflen(scmd);
@@ -3566,13 +3580,13 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 
 	/* We may not issue DMA commands if no DMA mode is set */
 	if (tf->protocol == ATA_PROT_DMA && !ata_dma_enabled(dev)) {
-		fp = 1;
+		fp = 1 + cdb_offset;
 		goto invalid_fld;
 	}
 
 	/* We may not issue NCQ commands to devices not supporting NCQ */
 	if (ata_is_ncq(tf->protocol) && !ata_ncq_enabled(dev)) {
-		fp = 1;
+		fp = 1 + cdb_offset;
 		goto invalid_fld;
 	}
 
@@ -3615,7 +3629,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	 */
 	if (tf->command == ATA_CMD_SET_FEATURES &&
 	    tf->feature == SETFEATURES_XFER) {
-		fp = (cdb[0] == ATA_16) ? 4 : 3;
+		fp = feature_offset;
 		goto invalid_fld;
 	}
 
@@ -3635,7 +3649,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	 * can turn off TC features of their system.
 	 */
 	if (tf->command >= 0x5C && tf->command <= 0x5F && !libata_allow_tpm) {
-		fp = (cdb[0] == ATA_16) ? 14 : 9;
+		fp = command_offset;
 		goto invalid_fld;
 	}

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

[Damien Le Moal]

On 11/14/26 02:48, 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,
> 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 <cassel@kernel.org>
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>

Reviewed-by: Damien Le Moal <dlemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research