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

[Damien Le Moal <dlemoal@kernel.org>]

On 2026/10/28 18:39, Zhao Gongyi wrote:
> 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.

Sure that can happen if the user sets up the SG_IO incorrectly.

> Translated commands can reach the same kind of mismatch. In particular,

No they cannot, as long as the kernel is correct for internally issued commands.
But t is not clear here if you are still talking about commands passed through
SG_IO.

> 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.

Why is this being checked here and not in the SCSI layer (sg driver and bsg)? It
seems sensible to check this there.

> 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.

Test details are not needed in the commit message.

> 
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Link: https://lore.kernel.org/r/20261027090809.94126-1-zhaogongyi@bytedance.com
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhao Gongyi <zhaogongyi@bytedance.com>
> ---
> 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);

The CDB may be perfectly correct. The error is in the SG_IO descriptor setup,
NOT in the CDB. So returning invalid field in CDB seems very misleading.
This is not something that SAT defines as far as I know, so we need to find a
sense code that is more informative. Please have a loo at
include/scsi/scsi_sense.h to see if something better exist.



> +		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);


-- 
Damien Le Moal
Western Digital Research