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

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

[shao.mingyin]

ATA PASS-THROUGH(32) uses the variable length CDB opcode 7Fh with
service action 1FF0h.  Commit b1ffbf854e08 added support for it, but
three paths still use the offsets of the 12- and 16-byte CDBs, or do
not recognize the 32-byte form at all.

Patch 1 fixes the PIO multi-sector sanity check and the multi_count
computation, which read cdb[1] instead of cdb[1 + cdb_offset], and
reports the field pointer for the same byte.

Patch 2 makes an unsupported service action fail with INVALID FIELD IN
CDB.  It currently returns 1 without setting sense data, so the command
completes with SAM_STAT_GOOD even though no ATA command was issued.

Patch 3 teaches the completion path to recognize ATA PASS-THROUGH(32)
and to read CK_COND from byte 11.  Without it, CK_COND is ignored and
the ATA Status Return sense descriptor is not built.

The three patches are independent and can be applied in any order.  All
were tested with a 32-byte CDB sent through SG_IO to a QEMU IDE disk;
the per-patch test descriptions are included after the --- line.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c

[PATCH 1/3] ata: libata-scsi: fix the multi_count check for 32-byte passthru

[shao.mingyin]

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

ata_scsi_pass_thru() translates 12, 16 and 32-byte ATA pass-through CDBs.
For a 32-byte CDB (VARIABLE_LENGTH_CMD with the ATA_32 service action) the
ATA command descriptor starts at byte offset 9, so the function adds
cdb_offset when reading the protocol (cdb[1 + cdb_offset]) and T_LENGTH
(cdb[2 + cdb_offset]) fields.

The PIO multi-sector sanity check and the multi_count computation,
however, still read cdb[1] without adding cdb_offset:

	if ((cdb[1] & 0xe0) && !is_multi_taskfile(tf))
	...
	unsigned int multi_count = 1 << (cdb[1] >> 5);

For a 32-byte CDB, cdb[1] is the variable length CDB control byte and
its bits 7:5 do not carry MULTIPLE_COUNT.  As a result, the sanity check
that rejects a multi-sector count on a non-multi taskfile is silently
skipped, and multi_count is always computed as 1 << 0 = 1, so the
comparison against dev->multi_count either emits a spurious
"invalid multi_count 1 ignored" warning for devices configured with a
different multi-sector size or misses a real mismatch.

Read the count field from cdb[1 + cdb_offset], matching the rest of the
function, and report the field pointer relative to the same byte so that
a rejected multi-sector count points at the MULTIPLE_COUNT field instead
of byte 1.  For 12 and 16-byte CDBs cdb_offset is zero, so their
behavior is unchanged.

Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
Tested on 7.3.0-next-20261030 (QEMU/TCG guest) with an ATA disk
(ata_piix / QEMU IDE) at /dev/sda.  A 32-byte ATA PASS-THROUGH(32)
CDB for a non-multi command (READ SECTORS) with the multi-sector
count field set (cdb[10] bits 7:5) is sent via SG_IO; a control CDB
with the count field clear is accepted on both kernels.

  unpatched                            patched
  command accepted,                    rejected with
  read completes (status GOOD)         CHECK CONDITION,
                                       ILLEGAL_REQUEST,
                                       INVALID FIELD IN CDB (0x5/0x24)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a61fb0331279..927e75b7498d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3574,13 +3574,13 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	}
 
 	/* sanity check for pio multi commands */
-	if ((cdb[1] & 0xe0) && !is_multi_taskfile(tf)) {
+	if ((cdb[1 + cdb_offset] & 0xe0) && !is_multi_taskfile(tf)) {
-		fp = 1;
+		fp = 1 + cdb_offset;
 		goto invalid_fld;
 	}
 
 	if (is_multi_taskfile(tf)) {
-		unsigned int multi_count = 1 << (cdb[1] >> 5);
+		unsigned int multi_count = 1 << (cdb[1 + cdb_offset] >> 5);
 
 		if (!dev->multi_count) {
 			ata_scsi_set_sense(dev, scmd, ABORTED_COMMAND,

Re: [PATCH 1/3] ata: libata-scsi: fix the multi_count check for 32-byte passthru

[Damien Le Moal]

On 11/4/26 23:09, shao.mingyin@zte.com.cn wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> ata_scsi_pass_thru() translates 12, 16 and 32-byte ATA pass-through CDBs.
> For a 32-byte CDB (VARIABLE_LENGTH_CMD with the ATA_32 service action) the
> ATA command descriptor starts at byte offset 9, so the function adds
> cdb_offset when reading the protocol (cdb[1 + cdb_offset]) and T_LENGTH
> (cdb[2 + cdb_offset]) fields.
> 
> The PIO multi-sector sanity check and the multi_count computation,
> however, still read cdb[1] without adding cdb_offset:
> 
> 	if ((cdb[1] & 0xe0) && !is_multi_taskfile(tf))
> 	...
> 	unsigned int multi_count = 1 << (cdb[1] >> 5);
> 
> For a 32-byte CDB, cdb[1] is the variable length CDB control byte and
> its bits 7:5 do not carry MULTIPLE_COUNT.  As a result, the sanity check
> that rejects a multi-sector count on a non-multi taskfile is silently
> skipped, and multi_count is always computed as 1 << 0 = 1, so the
> comparison against dev->multi_count either emits a spurious
> "invalid multi_count 1 ignored" warning for devices configured with a
> different multi-sector size or misses a real mismatch.
> 
> Read the count field from cdb[1 + cdb_offset], matching the rest of the
> function, and report the field pointer relative to the same byte so that
> a rejected multi-sector count points at the MULTIPLE_COUNT field instead
> of byte 1.  For 12 and 16-byte CDBs cdb_offset is zero, so their
> behavior is unchanged.
> 
> Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>

Looks OK.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

[PATCH 2/3] 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() handles a variable length CDB only when its
service action is ATA_32 (ATA PASS-THROUGH (32)).  For any other service
action it returns 1, which tells ata_scsi_translate() that the command
has been handled and that scmd->result and the sense buffer were set by
the translation function.

No sense data is set in that path, so ata_scsi_translate() frees the qc
and calls scsi_done() with scmd->result still zero.  The command thus
completes with SAM_STAT_GOOD even though no ATA command was issued and
no data was transferred.  Before ATA PASS-THROUGH (32) support was added,
such CDBs were not claimed by ata_get_xlat_func() and were rejected by
ata_scsi_simulate() with ILLEGAL REQUEST/INVALID COMMAND OPERATION CODE.

Fail unsupported service actions with ILLEGAL REQUEST/INVALID FIELD IN
CDB and point the field pointer at the SERVICE ACTION field.

Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
Tested on 7.3.0-next-20261030 (QEMU/TCG guest) with an ATA disk
(ata_piix, /dev/sda); no special hardware is needed.

A 32-byte VARIABLE_LENGTH_CMD CDB with service action 0x1234 (not
ATA_32) and a 512-byte data-in buffer is sent with SG_IO.  A valid
ATA PASS-THROUGH(32) READ SECTORS CDB is the control and is accepted
on both kernels.

  unpatched                            patched
  status 0x00 (SAM_STAT_GOOD)          status 0x02 (CHECK CONDITION)
                                       sk 0x05, asc 0x24
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a61fb0331279..d29b54688b53 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -5027,7 +5027,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;
 }

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

[Damien Le Moal]

On 11/4/26 23:11, shao.mingyin@zte.com.cn wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> ata_scsi_var_len_cdb_xlat() handles a variable length CDB only when its
> service action is ATA_32 (ATA PASS-THROUGH (32)).  For any other service
> action it returns 1, which tells ata_scsi_translate() that the command
> has been handled and that scmd->result and the sense buffer were set by
> the translation function.
> 
> No sense data is set in that path, so ata_scsi_translate() frees the qc
> and calls scsi_done() with scmd->result still zero.  The command thus
> completes with SAM_STAT_GOOD even though no ATA command was issued and
> no data was transferred.  Before ATA PASS-THROUGH (32) support was added,
> such CDBs were not claimed by ata_get_xlat_func() and were rejected by
> ata_scsi_simulate() with ILLEGAL REQUEST/INVALID COMMAND OPERATION CODE.
> 
> Fail unsupported service actions with ILLEGAL REQUEST/INVALID FIELD IN
> CDB and point the field pointer at the SERVICE ACTION field.
> 
> Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>

Looks OK.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

[PATCH 3/3] 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() detects ATA pass-through commands by the ATA_12
and ATA_16 opcodes only.  ATA PASS-THROUGH (32) uses the variable
length CDB opcode 7Fh with service action ATA_32, so for those commands
the function takes the ordinary completion path: the CK_COND bit is
never honored and the ATA Status Return sense descriptor is never
built.  A successful command with CK_COND set is reported as GOOD
instead of CHECK CONDITION, and a failed command gets the generic ATA
sense data without the ATA register values.

The byte that carries T_LENGTH and CK_COND is byte 2 for 12- and
16-byte CDBs, but byte 11 for a 32-byte CDB, whose byte 2 is part of the
variable length CDB header.  Detect ATA PASS-THROUGH (32) through its
service action and read CK_COND from the correct byte.

Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
Cc: stable@vger.kernel.org
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
Tested on 7.3.0-next-20261030 (QEMU/TCG guest) with an ATA disk
(ata_piix, /dev/sda); no special hardware is needed.

A 32-byte ATA PASS-THROUGH(32) READ SECTORS CDB with CK_COND set in
cdb[11] and a 512-byte data-in buffer is sent with SG_IO.  The same
CDB with CK_COND clear is the control and completes GOOD on both.

  unpatched                            patched
  status 0x00, no sense data           status 0x02 (CHECK CONDITION)
                                       sk 0x01, asc/ascq 0x00/0x1d
                                       ATA descriptor 0x09 present
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a61fb0331279..97adeecc7736 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
@@ -3389,6 +3392,6 @@ 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;
 	}

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

[Damien Le Moal]

On 11/4/26 23:11, shao.mingyin@zte.com.cn wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> ata_scsi_qc_complete() detects ATA pass-through commands by the ATA_12
> and ATA_16 opcodes only.  ATA PASS-THROUGH (32) uses the variable
> length CDB opcode 7Fh with service action ATA_32, so for those commands
> the function takes the ordinary completion path: the CK_COND bit is
> never honored and the ATA Status Return sense descriptor is never
> built.  A successful command with CK_COND set is reported as GOOD
> instead of CHECK CONDITION, and a failed command gets the generic ATA
> sense data without the ATA register values.
> 
> The byte that carries T_LENGTH and CK_COND is byte 2 for 12- and
> 16-byte CDBs, but byte 11 for a 32-byte CDB, whose byte 2 is part of the
> variable length CDB header.  Detect ATA PASS-THROUGH (32) through its
> service action and read CK_COND from the correct byte.
> 
> Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>

Looks OK.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research