Re: [PATCH] ata: libata-scsi: limit simulated SCSI command copy to response length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] A malicious or misconfigured ATA device can trigger an out-of-bounds heap read in ata_dev_config_cpr() and a massive global buffer overflow in ata_scsiop_inq_b9() by reporting a large nr_cpr value. The newly added defensive check in ata_scsi_rbuf_fill() executes after the overflow has already occurred.
- [Critical] ata_format_dsm_trim_descr() overflows the static ata_scsi_rbuf for devices with large sector sizes.
--

commit 8b4a8e5bfe8806f87d80466e94aa7a4684517178
Author: Karuna Ramkumar <rkaruna@google.com>

ata: libata-scsi: limit simulated SCSI command copy to response length

The function ata_scsi_rbuf_fill() is used to copy the response of
emulated SCSI commands from ata_scsi_rbuf to the SCSI command's
scatterlist.

[ ... ]

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d54ec1631e9a6..0ddec111a78c4 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1933,8 +1933,10 @@ static void ata_scsi_rbuf_fill(struct ata_device *dev, struct scsi_cmnd *cmd,
>  	memset(ata_scsi_rbuf, 0, ATA_SCSI_RBUF_SIZE);
>  	len = actor(dev, cmd, ata_scsi_rbuf);
>  	if (len) {
> +		if (WARN_ON(len > ATA_SCSI_RBUF_SIZE))
> +			len = ATA_SCSI_RBUF_SIZE;

[Severity: Critical]
This is a pre-existing issue, but does the overflow already occur before this
check is reached?

The actor function writes directly into the static ata_scsi_rbuf array.
For example, in ata_scsiop_inq_b9():

    for (i = 0; i < cpr_log->nr_cpr; i++, desc += 32) {
        ...
        put_unaligned_be64(cpr_log->cpr[i].num_lbas, &desc[16]);
    }

If a device reports a large nr_cpr value (up to 255), this loop could write
up to 8224 bytes into the 2048-byte ata_scsi_rbuf array during the actor()
call, before the defensive WARN_ON() check has a chance to execute.

Additionally, could ata_dev_config_cpr() perform an out-of-bounds heap read
when reading this large nr_cpr value?

    cpr_log->nr_cpr = nr_cpr;
    desc = &buf[64];
    for (i = 0; i < nr_cpr; i++, desc += 32) {
        cpr_log->cpr[i].num = desc[0];
        ...

If nr_cpr exceeds the allocated buffer space based on buf_len, this loop
appears to read past the end of the allocation.


[Severity: Critical]
This is also a pre-existing issue, but does ata_format_dsm_trim_descr()
overflow this same static buffer for devices with large sector sizes?

When a filesystem discard is issued to a device with a 4K logical sector size,
ata_scsi_write_same_xlat() calculates trmax as 512:

    size_t len = sdp->sector_size;
    ...
    const u32 trmax = len >> 3;

In ata_format_dsm_trim_descr():

    if (len > ATA_SCSI_RBUF_SIZE)
        len = ATA_SCSI_RBUF_SIZE;
    ...
    memset(buf, 0, len);
    while (i < trmax) {
        ...
        buf[i++] = __cpu_to_le64(entry);

While the memset() length is properly capped to 2048 bytes, the while loop
continues up to trmax. For a trmax of 512, this writes 512 8-byte entries
(4096 bytes) into the 2048-byte ata_scsi_rbuf array.

>  		sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
> -				    ata_scsi_rbuf, ATA_SCSI_RBUF_SIZE);
> +				    ata_scsi_rbuf, len);
>  		cmd->result = SAM_STAT_GOOD;
>  		if (scsi_bufflen(cmd) > len)
>  			scsi_set_resid(cmd, scsi_bufflen(cmd) - len);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629175024.2187500-1-rkaruna@google.com?part=1