Re: [PATCH 2/2] ata: libata-scsi: Bound the VPD B9h ranges to the response buffer

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

On 6/20/26 11:36, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> ata_scsiop_inq_b9() writes one 32-byte range descriptor per
> cpr_log->nr_cpr into the fixed 2048-byte ata_scsi_rbuf with no bound.
> The count originates from the device (concurrent positioning ranges log
> 0x47, up to 255), so an INQUIRY VPD page B9h to a drive whose log
> advertises more than 62 ranges overflows the static buffer by up to 6168
> bytes: a global out-of-bounds write into adjacent kernel data.
> 
> Emit only as many descriptors as the response buffer can hold and size
> the page length to match. The companion core fix bounds the stored count
> against the device log, but a large self-consistent log still yields
> nr_cpr up to 255, so the emitter needs its own bound.
> 
> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---
>  drivers/ata/libata-scsi.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d43207c6e467..d43306c5d375 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2355,18 +2355,24 @@ static unsigned int ata_scsiop_inq_b9(struct ata_device *dev,
>  {
>  	struct ata_cpr_log *cpr_log = dev->cpr_log;
>  	u8 *desc = &rbuf[64];
> -	int i;
> +	int i, nr_cpr;
>  
>  	if (!cpr_log) {
>  		ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
>  		return 0;
>  	}
>  
> +	/*
> +	 * The page is built in the fixed-size ata_scsi_rbuf, so the number of
> +	 * range descriptors returned is bounded by that buffer's size.
> +	 */
> +	nr_cpr = min_t(int, cpr_log->nr_cpr, (ATA_SCSI_RBUF_SIZE - 64) / 32);
> +

I do not think this is the right place to fix this. Rather, we should define the
maximum number of ranges we can support given a 2K rbuf, that is, (2048 - 64) /
32 = 62, and check on probe that we are not actually exceeding that value when
scanning the disk. If we do, warn and ignore CPR.

62 ranges would mean 62 actuators... We are light years away from seeing drives
with that many actuators (we are lucky if we get 2, so...).

So I think we can drop this patch and simply reinforce the checks on the number
of cpr in patch 1. Or make this additional patch another fix if you prefer.
Either way is OK with me.

>  	/* 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(64 + nr_cpr * 32 - 4, &rbuf[2]);
>  
> -	for (i = 0; i < cpr_log->nr_cpr; i++, desc += 32) {
> +	for (i = 0; i < 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]);
> 


-- 
Damien Le Moal
Western Digital Research