[PATCH v2] ata: libata-scsi: reject data-in commands with a data-out buffer

[Zhao Gongyi]

ata_scsi_rbuf_fill() copies simulated data-in replies to the SCSI
scatterlist without checking the request direction. SG_IO can supply a
data-in CDB with DMA_TO_DEVICE, in which case the pages may have been
pinned without write access. In the reported case, copying the reply
into the shared zero page caused a kernel page fault.

Translated commands can reach the same kind of mismatch. In particular,
the ZBC IN and GET PHYSICAL ELEMENT STATUS completion handlers rewrite
the receive buffer, including when the device aborts the command.
Checking the direction only in ata_scsi_rbuf_fill() leaves those paths
unprotected.

Reject DMA_TO_DEVICE requests in the simulated-command path. After
translation, reject a DMA_TO_DEVICE request whose ATA taskfile performs
a data-in transfer, before issuing it to the device. Return ILLEGAL
REQUEST / INVALID FIELD IN CDB for both cases.

The original reproducer panicked in ata_scsi_rbuf_fill() on all four
runs with the unpatched kernel. With this patch applied, it completed
without a crash. A diagnostic run confirmed CHECK CONDITION with
ILLEGAL REQUEST / INVALID FIELD IN CDB.

Built an x86_64 defconfig kernel from the current Linus tree and booted it
in QEMU with an ata_piix disk. Normal INQUIRY and READ(10) SG_IO requests
succeeded. Each command declared as SG_DXFER_TO_DEV returned CHECK
CONDITION with ILLEGAL REQUEST / INVALID FIELD IN CDB. No kernel
warnings or oopses appeared. ZBC IN and GET PHYSICAL ELEMENT STATUS
were checked by code inspection; the test disk does not support them.

Suggested-by: Niklas Cassel <cassel@xxxxxxxxxx>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Link: https://lore.kernel.org/r/20261027090809.94126-1-zhaogongyi@xxxxxxxxxxxxx
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Zhao Gongyi <zhaogongyi@xxxxxxxxxxxxx>
---
Changes since v1:
  - Rebase on Linus' tree and use INVALID_FIELD_IN_CDB with the current
    ata_scsi_set_sense() signature.
  - Reject mismatched translated data-in commands before issuing them,
    as suggested by Niklas Cassel.
  - Remove the deprecated Assisted-by trailer.

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a61fb0331..a5170a36a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2048,6 +2048,19 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
 	if (xlat_func(qc))
 		goto done;
 
+	/*
+	 * A data-out request may contain pages pinned without write access.
+	 * Reject a translated data-in command before issuing it, since its
+	 * completion handler may write to the buffer even if the device fails.
+	 */
+	if (unlikely(cmd->sc_data_direction == DMA_TO_DEVICE &&
+		     ata_is_data(qc->tf.protocol) &&
+		     !(qc->tf.flags & ATA_TFLAG_WRITE))) {
+		ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST,
+				   INVALID_FIELD_IN_CDB);
+		goto done;
+	}
+
 	return ata_scsi_qc_issue(ap, qc);
 
 done:
@@ -2079,6 +2092,13 @@ static void ata_scsi_rbuf_fill(struct ata_device *dev, struct scsi_cmnd *cmd,
 	unsigned long flags;
 	unsigned int len;
 
+	/* A data-out request may contain pages pinned without write access. */
+	if (cmd->sc_data_direction == DMA_TO_DEVICE) {
+		ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST,
+				   INVALID_FIELD_IN_CDB);
+		return;
+	}
+
 	spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
 
 	memset(ata_scsi_rbuf, 0, ATA_SCSI_RBUF_SIZE);
-- 
2.39.5 (Apple Git-154)