Re: [PATCH v2] ata: libata-core: Reject an invalid concurrent positioning ranges count

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

On 6/23/26 03:46, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The patch strictly rejects the entire Concurrent Positioning Ranges (CPR) log if the range count exceeds 62 or the returned log size, permanently disabling the feature for otherwise compliant devices.
> --
> 
> commit fb7353b474c07b1173b746d017d47feac25f57b1
> Author: Bryam Vargas <hexlabsecurity@proton.me>
> 
> ata: libata-core: Reject an invalid concurrent positioning ranges count
> 
> This commit adds bounds checking for the concurrent positioning ranges (CPR)
> log count. It rejects the log if the count exceeds 62 (the maximum capacity
> of the VPD B9h buffer) or if the count exceeds the returned log size.
> 
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 3d0027ec33c2a..017a16be158b6 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2832,6 +2832,18 @@ static void ata_dev_config_cpr(struct ata_device *dev)
>>  	if (!nr_cpr)
>>  		goto out;
>>  
>> +	/*
>> +	 * The count is reported independently of the log size and is also
>> +	 * emitted into the fixed 2048-byte VPD B9h buffer, which holds at most
>> +	 * (2048 - 64) / 32 = 62 descriptors. Reject a count that exceeds that
>> +	 * or does not fit the log the device returned.
>> +	 */
>> +	if (nr_cpr > 62 || buf_len < 64 + (size_t)nr_cpr * 32) {
>> +		ata_dev_warn(dev,
>> +			     "Invalid number of concurrent positioning ranges\n");
>> +		goto out;
>> +	}
> 
> [Severity: Medium]
> Does this code completely disable the CPR feature for spec-compliant devices
> that report more than 62 ranges?
> 
> The ATA specification allows up to 255 concurrent positioning ranges, and the
> 62-descriptor limit seems to be an internal Linux SCSI translation limit based
> on the fixed 2048-byte ata_scsi_rbuf size. By jumping to out, we discard the
> log entirely rather than capping it to fit internal limits.

Bryan, I think we can ignore this since again, more than a few CPRs is simply
not realistic in practice. But update the code comment to mention that the specs
allow up to 255 CPRs but that we limit this to what fits in the RBUF for
emitting the VPD page. And maybe we should have:

/*
 * Maximum number of concurrent positioning ranges supported. The ACS
 * specifications allow up to 255, but we limit this to what fits in the
 * internal rbuf buffer used to emit VPD page B9h.
 */
#define ATA_DEV_MAX_CPR		min(255, ((ATA_SCSI_RBUF_SIZE - 64) / 32))

> 
> Also, the commit message states "Capping the stored count keeps the emitter in
> bounds", but the code appears to reject the log outright instead of capping
> the count. Could we instead dynamically cap nr_cpr to 62 here, so that devices
> with more ranges still get partial functionality rather than having the feature
> disabled entirely?

Yes, we do not cap but reject. Let's update the comment.

> 
>> +
>>  	cpr_log = kzalloc_flex(*cpr_log, cpr, nr_cpr);
>>  	if (!cpr_log)
>>  		goto out;
> 


-- 
Damien Le Moal
Western Digital Research