Re: [PATCH] ata: libata-scsi: add out-of-bounds checks in ata_scsiop_inq_b9

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly added bounds check to prevent `page_len > ATA_SCSI_RBUF_SIZE` is mathematically dead code.
--

--- Patch [1]: [PATCH] ata: libata-scsi: add out-of-bounds checks in ata_scsiop_inq_b9 ---
commit bacad4b494e8c54b7f847f50ed2b764d2bdf3fde
Author: Karuna Ramkumar <[email protected]>
ata: libata-scsi: add out-of-bounds checks in ata_scsiop_inq_b9

This commit adds a bounds check to ata_scsiop_inq_b9() to ensure the
calculated page length does not exceed the SCSI request buffer size. If the
calculated size is larger, the command is aborted to prevent a buffer
overflow.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> @@ -2361,15 +2361,22 @@ static unsigned int ata_scsiop_inq_b9(struct ata_device *dev,
[ ... ]
>  	if (!cpr_log) {
>  		ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
>  		return 0;
>  	}
>  
> +	page_len = 64 + (int)cpr_log->nr_cpr * 32;
> +	if (page_len > ATA_SCSI_RBUF_SIZE) {
> +		ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
> +		return 0;
> +	}

[Severity: Low]
Is this bounds check reachable during normal operation?

Looking at how cpr_log is populated in ata_read_cpr_log(), nr_cpr is
already explicitly validated against ATA_DEV_MAX_CPR. Since ATA_DEV_MAX_CPR
is mathematically derived from ATA_SCSI_RBUF_SIZE, a device reporting a
value exceeding this limit has its log discarded, leaving dev->cpr_log as
NULL.

Because ata_scsiop_inq_b9() immediately returns if cpr_log is NULL right
above this snippet, it seems page_len can never evaluate to a value greater
than ATA_SCSI_RBUF_SIZE. Does this make the new check mathematically dead
code under real inputs?

> +
>  	/* SCSI Concurrent Positioning Ranges VPD page: SBC-5 rev 1 or later */
>  	rbuf[1] = 0xb9;
> -	put_unaligned_be16(64 + (int)cpr_log->nr_cpr * 32 - 4, &rbuf[2]);
> +	put_unaligned_be16(page_len - 4, &rbuf[2]);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1