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

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

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

ata_dev_config_cpr() takes the number of range descriptors from buf[0]
of the concurrent positioning ranges log (up to 255), which the device
reports independently of the log size in the GPL directory. The count is
then walked at a fixed 32-byte stride in two places with no bound: the
log read here, and the INQUIRY VPD page B9h emitter, which writes one
descriptor per range into the fixed 2048-byte ata_scsi_rbuf. A device
reporting a count larger than its own log overflows the read buffer (up
to 7704 bytes past a 512-byte slab), and a count above 62 overflows the
response buffer on the emit side.

Bound the count once, on probe, against both the log the device returned
and the 62-descriptor capacity of the VPD B9h buffer; warn and ignore the
log when it does not fit. Capping the stored count keeps the emitter in
bounds with no separate change there.

Suggested-by: Damien Le Moal <dlemoal@kernel.org>
Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
Fixes: c745dfc541e7 ("libata: fix reading concurrent positioning ranges log")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: Per Damien Le Moal's review, reject an inconsistent count instead of
    clamping it, and fold the emitter bound into this single probe-time
    check: a count above 62 (the VPD B9h 2048-byte buffer capacity) or one
    that does not fit the device's own log is rejected with a warning. That
    makes the separate emitter patch unnecessary, so v1 2/2 is dropped.
    v1 1/2: https://lore.kernel.org/all/20260619-b4-disp-6200c44e-v1-1-4624a4707d9e@proton.me/
    v1 2/2: https://lore.kernel.org/all/20260619-b4-disp-6200c44e-v1-2-4624a4707d9e@proton.me/

A/B with a userspace AddressSanitizer mirror of both sinks (-m64 and -m32;
the unbounded value is device log content, not bus state, so no hardware is
needed):

  count  buf_len  with this patch
  2      128      accepted (conforming drive)
  62     2048     accepted (62 actuators, the documented maximum)
  63     8704     rejected (exceeds the 62-descriptor VPD B9h buffer)
  255    8704     rejected (self-consistent log, but the emit would overflow)
  62     512      rejected (count does not fit the device's own log)
  255    512      rejected

Without the patch the 512-byte rows fault on the log read and the >62 rows
fault on the VPD B9h emit; with it every count the emitter can still receive
fits the 2048-byte buffer. Reproducer available on request.
---
 drivers/ata/libata-core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3d0027ec33c2..017a16be158b 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;
+	}
+
 	cpr_log = kzalloc_flex(*cpr_log, cpr, nr_cpr);
 	if (!cpr_log)
 		goto out;

---
base-commit: 4549871118cf616eecdd2d939f78e3b9e1dddc48
change-id: 20260622-b4-disp-5734c6f2-8f8c307f8bf1

Best regards,
-- 
bryamzxz <hexlabsecurity@proton.me>

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

[sashiko-bot]

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.

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-b4-disp-5734c6f2-v2-1-53083c2df3c6@proton.me?part=1

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

[Damien Le Moal]

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

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

[Damien Le Moal]

On 6/23/26 03:38, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> ata_dev_config_cpr() takes the number of range descriptors from buf[0]
> of the concurrent positioning ranges log (up to 255), which the device
> reports independently of the log size in the GPL directory. The count is
> then walked at a fixed 32-byte stride in two places with no bound: the
> log read here, and the INQUIRY VPD page B9h emitter, which writes one
> descriptor per range into the fixed 2048-byte ata_scsi_rbuf. A device
> reporting a count larger than its own log overflows the read buffer (up
> to 7704 bytes past a 512-byte slab), and a count above 62 overflows the
> response buffer on the emit side.
> 
> Bound the count once, on probe, against both the log the device returned
> and the 62-descriptor capacity of the VPD B9h buffer; warn and ignore the
> log when it does not fit. Capping the stored count keeps the emitter in
> bounds with no separate change there.
> 
> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
> Fixes: c745dfc541e7 ("libata: fix reading concurrent positioning ranges log")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---
> v2: Per Damien Le Moal's review, reject an inconsistent count instead of
>     clamping it, and fold the emitter bound into this single probe-time
>     check: a count above 62 (the VPD B9h 2048-byte buffer capacity) or one
>     that does not fit the device's own log is rejected with a warning. That
>     makes the separate emitter patch unnecessary, so v1 2/2 is dropped.
>     v1 1/2: https://lore.kernel.org/all/20260619-b4-disp-6200c44e-v1-1-4624a4707d9e@proton.me/
>     v1 2/2: https://lore.kernel.org/all/20260619-b4-disp-6200c44e-v1-2-4624a4707d9e@proton.me/
> 
> A/B with a userspace AddressSanitizer mirror of both sinks (-m64 and -m32;
> the unbounded value is device log content, not bus state, so no hardware is
> needed):
> 
>   count  buf_len  with this patch
>   2      128      accepted (conforming drive)
>   62     2048     accepted (62 actuators, the documented maximum)
>   63     8704     rejected (exceeds the 62-descriptor VPD B9h buffer)
>   255    8704     rejected (self-consistent log, but the emit would overflow)
>   62     512      rejected (count does not fit the device's own log)
>   255    512      rejected
> 
> Without the patch the 512-byte rows fault on the log read and the >62 rows
> fault on the VPD B9h emit; with it every count the emitter can still receive
> fits the 2048-byte buffer. Reproducer available on request.
> ---
>  drivers/ata/libata-core.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 3d0027ec33c2..017a16be158b 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;
> +	}

Yes, this is much nicer. However, the hardcoded 62 is not great. If we ever
change the rbuf size, this will be wrong and we will have a hard time noticing
it. So let's do this properly and define something like:

#define ATA_DEV_MAX_CPR	((ATA_SCSI_RBUF_SIZE - 64) / 32)

in drivers/ata/libata.h
That requires moving the definition of ATA_SCSI_RBUF_SIZE for libata-scsi.c to
libata.h, which is fine.

Also, we may want to separate the checks to have a different warning:

	if (nr_cpr > ATA_DEV_MAX_CPR) {
		ata_dev_warn(dev,
			"Too many concurrent positioning ranges\n");
		goto out;
	}

	if (buf_len < 64 + (size_t)nr_cpr * 32) {
		ata_dev_warn(dev,
			"Invalid number of concurrent positioning ranges\n");
		goto out;
	}


> +
>  	cpr_log = kzalloc_flex(*cpr_log, cpr, nr_cpr);
>  	if (!cpr_log)
>  		goto out;
> 
> ---
> base-commit: 4549871118cf616eecdd2d939f78e3b9e1dddc48
> change-id: 20260622-b4-disp-5734c6f2-8f8c307f8bf1
> 
> Best regards,


-- 
Damien Le Moal
Western Digital Research