Re: [PATCH] ata: libata-scsi: reject non-PIO protocols for READ/WRITE MULTIPLE

[Niklas Cassel <cassel@kernel.org>]

Hello Zhao,

On Wed, Oct 28, 2026 at 10:04:32PM +0800, Zhao Gongyi wrote:
> From: Zhao Gongyi <zhaogongyi@BYTEDANCE.COM>
> 
> The commands recognized by is_multi_taskfile() are ATA READ/WRITE
> MULTIPLE variants, which transfer data using PIO. ata_scsi_pass_thru()
> validates the MULTIPLE_COUNT field for these commands, but accepts any
> protocol selected by the ATA PASS-THROUGH CDB.
> 
> This allows userspace, for example, to combine WRITE MULTIPLE with the
> DMA protocol. With a QEMU PIIX disk, the device enters its PIO data phase
> and waits with DRQ asserted while the host waits for DMA completion. The
> request remains active until the user-selected command timeout. Since
> the disk has a single tag, filesystem writeback and later SG_IO requests
> block behind it.
> 
> Require the protocol to map to ATA_PROT_PIO when is_multi_taskfile()
> recognizes the command. Reject any other protocol as an invalid CDB
> field before issuing the taskfile. Keep the check local to the existing
> READ/WRITE MULTIPLE handling so pass-through behavior for other ATA
> commands is unchanged.
> 
> Fixes: 1dce589c38c3 ("libata passthru: support PIO multi commands")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhao Gongyi <zhaogongyi@BYTEDANCE.COM>
> ---
> Tested on a QEMU PIIX disk with 16-sector transfers and a multiple count
> of 16:
> 
> - WRITE MULTIPLE with DMA was rejected with ILLEGAL REQUEST/INVALID
>   FIELD IN CDB and a field pointer of 1.
> - READ MULTIPLE with DMA was rejected with the same sense data.
> - WRITE MULTIPLE with PIO data-out completed successfully.
> - READ MULTIPLE with PIO data-in completed successfully and returned the
>   8192 bytes written by the preceding command.
> 
> Both invalid commands completed in less than 1 ms. No ATA timeout or
> hung-task warning was reported, and subsequent I/O remained responsive.
> The modified libata-scsi.o also builds successfully.
> 
>  drivers/ata/libata-scsi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 115b75a313..31d822d78e 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3507,6 +3507,11 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>  	if (is_multi_taskfile(tf)) {
>  		unsigned int multi_count = 1 << (cdb[1] >> 5);
>  
> +		if (tf->protocol != ATA_PROT_PIO) {
> +			fp = 1 + cdb_offset;
> +			goto invalid_fld;
> +		}
> +
>  		if (!dev->multi_count) {
>  			ata_scsi_set_sense(dev, scmd, ABORTED_COMMAND, 0, 0);
>  			return 1;
> 

I'm a bit conflicted about this patch.

There is nothing in SAT specs that requires us to validate the protocol field.
When using normal (non-passthrough commands), the protocol field is initialized
by the regular SCSI to ATA translation done by libata.

For passthrough commands, it comes from the user.

Not only does it come from the user, it requires CAP_SYS_RAWIO:

scsi_cmd_allowed() {
        if (capable(CAP_SYS_RAWIO))
                return true;
    ...


(ATA_12 / ATA_16 / ATA_32 will all return false, so not allowed for a regular
user.)

And... even if the user sends a command with a protocol that is not as per
ACS spec, the command should time out and EH should reset the device.


So... If the user sends a passthrough command with the wrong protocol...
Well, that is on the user, no?
(It also requires CAP_SYS_RAWIO)


If we were to add protocol checks all passthrough commands, then we should
not only check READ/WRITE MULTIPLE, we should check other commands like
WRITE SECTORS (30h) and IDENTIFY DEVICE (ECh) too.

But, I don't think we want that. Damien, thoughts?


Kind regards,
Niklas