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

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

Pre-existing issues:
- [Critical] The defensive bounds check `WARN_ON(len > ATA_SCSI_RBUF_SIZE)` is ineffective because it evaluates after the `actor` callback has already performed an out-of-bounds write.
--

commit 41acccb75ba899e5e72a927d95a97d5d6f088c63
Author: Karuna Ramkumar <[email protected]>

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

This patch limits the sg_copy_from_buffer() size to the actual response length
instead of the full buffer size to avoid overwriting the tail of the transfer
buffer. It also adds a bounds check on the length returned by the actor
function to abort if it exceeds the static buffer capacity.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d54ec1631e9a6..836981b176c7d 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1933,8 +1933,13 @@ 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)) {
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Critical]
This is a pre-existing issue, but does this defensive bounds check actually
prevent memory corruption?

Because the check evaluates after the actor callback has already executed,
the actor function may have already performed an out-of-bounds write. For
example, a malicious or buggy ATA device reporting a large number of
Concurrent Positioning Ranges (nr_cpr up to 255) could cause
ata_scsiop_inq_b9() to write up to 8224 bytes into the 2048-byte statically
allocated ata_scsi_rbuf array:

drivers/ata/libata-scsi.c:ata_scsiop_inq_b9() {
	...
	for (i = 0; i < cpr_log->nr_cpr; i++, desc += 32) {
		desc[0] = cpr_log->cpr[i].num;
		desc[1] = cpr_log->cpr[i].num_storage_elements;
		put_unaligned_be64(cpr_log->cpr[i].start_lba, &desc[8]);
		put_unaligned_be64(cpr_log->cpr[i].num_lbas, &desc[16]);
	}
	...
}

Should the actor functions be modified to take a buffer size parameter so
they can safely enforce limits before writing memory?

> +			ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
> +			spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
> +			return;
> +		}
>  		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/[email protected]?part=1