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

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

[Zhao Gongyi]

SG_IO can submit a data-in SCSI command with DMA_TO_DEVICE. The
scatterlist can then contain pages pinned without write access.
ata_scsi_rbuf_fill() writes simulated replies to that scatterlist;
in the reported case, the copy tried to write the shared zero page
and faulted in the kernel.

The same mismatch can affect SG_IO commands translated by libata.
ZBC IN and GET PHYSICAL ELEMENT STATUS completion handlers can modify
the receive buffer even if the drive aborts the command. For ATAPI,
the successful INQUIRY completion also rewrites the buffer in
atapi_fixup_inquiry().

Reject these mismatched commands before the buffer is written or the
command is issued. Report ABORTED COMMAND / DATA IN BUFFER ERROR because
the CDB itself can be valid; the supplied transfer direction is wrong.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Link: https://lore.kernel.org/r/20261027090809.94126-1-zhaogongyi@bytedance.com
Suggested-by: Niklas Cassel <cassel@kernel.org>
Suggested-by: Damien Le Moal <dlemoal@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: Zhao Gongyi <zhaogongyi@bytedance.com>
---
Changes since v2:
  - Reject misdirected ATAPI INQUIRY before issuing the PACKET command;
    its completion fixup also writes to the scatterlist (Niklas Cassel).
  - Use ABORTED_COMMAND / DATA_IN_BUFFER_ERROR for all rejection paths,
    following Damien Le Moal's suggestion.
  - Clarify that the translated commands are SG_IO submissions and move
    detailed test results below the separator.

Previous v2 validation:
  - The original reproducer panicked in ata_scsi_rbuf_fill() on all four
    unpatched runs and completed without a crash with v2 applied.
  - On x86_64 QEMU with an ata_piix disk, correctly directed INQUIRY and
    READ(10) succeeded; misdirected requests returned CHECK CONDITION.
  - ZBC IN and GET PHYSICAL ELEMENT STATUS were inspected in code; the
    QEMU test disk did not support those commands.

New v3 validation:
  - On upstream master 05441d697f7f, libata-scsi.o and bzImage built.
  - On x86_64 QEMU with an ATAPI DVD-ROM, a standard INQUIRY sent with
    SG_DXFER_TO_DEV and a read-only 4 KiB buffer returned GOOD on the
    unpatched kernel. With v3, it returned CHECK CONDITION with
    ABORTED COMMAND / DATA IN BUFFER ERROR. Neither run produced an Oops.
  - The patch applies to the same libata-scsi.c blob used for v2
    (a61fb0331279).

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a61fb03..78a05cc 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, ABORTED_COMMAND,
+				   DATA_IN_BUFFER_ERROR);
+		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, ABORTED_COMMAND,
+				   DATA_IN_BUFFER_ERROR);
+		return;
+	}
+
 	spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
 
 	memset(ata_scsi_rbuf, 0, ATA_SCSI_RBUF_SIZE);
@@ -3173,6 +3193,14 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 
 	qc->complete_fn = atapi_qc_complete;
 
+	/* atapi_fixup_inquiry() writes to the buffer on successful completion. */
+	if (scmd->sc_data_direction == DMA_TO_DEVICE &&
+	    scmd->cmnd[0] == INQUIRY) {
+		ata_scsi_set_sense(dev, scmd, ABORTED_COMMAND,
+				   DATA_IN_BUFFER_ERROR);
+		return 1;
+	}
+
 	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	if (scmd->sc_data_direction == DMA_TO_DEVICE) {
 		qc->tf.flags |= ATA_TFLAG_WRITE;

base-commit: 65ba8f34ec886ec72b2c07ddc8cf3d815c32869d
-- 
2.39.5 (Apple Git-154)

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

[Damien Le Moal]

On 11/4/26 22:06, Zhao Gongyi wrote:
> SG_IO can submit a data-in SCSI command with DMA_TO_DEVICE. The
> scatterlist can then contain pages pinned without write access.
> ata_scsi_rbuf_fill() writes simulated replies to that scatterlist;
> in the reported case, the copy tried to write the shared zero page
> and faulted in the kernel.
> 
> The same mismatch can affect SG_IO commands translated by libata.
> ZBC IN and GET PHYSICAL ELEMENT STATUS completion handlers can modify
> the receive buffer even if the drive aborts the command. For ATAPI,
> the successful INQUIRY completion also rewrites the buffer in
> atapi_fixup_inquiry().
> 
> Reject these mismatched commands before the buffer is written or the
> command is issued. Report ABORTED COMMAND / DATA IN BUFFER ERROR because
> the CDB itself can be valid; the supplied transfer direction is wrong.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Link: https://lore.kernel.org/r/20261027090809.94126-1-zhaogongyi@bytedance.com
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhao Gongyi <zhaogongyi@bytedance.com>

Looks OK. If needed, we can tweak the sense code later.

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

-- 
Damien Le Moal
Western Digital Research