Re: [PATCH bpf-next v2 05/13] bpf: keep the range across a sign extension that cannot change it

[Vineet Gupta <vineet.gupta@linux.dev>]

On 9/10/26 10:08 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Medium] Incomplete fix for bounds precision loss on zero-straddling sign extensions in the verifier.
>
> Pre-existing issues:
> - [High] Out-of-bounds array read in BPF memory cgroup kfuncs due to bypassable enum bounds check.
> --
>
> commit 971a50869c707d4ac0627b912bd2423344987acc
> Author: Vineet Gupta <vineet.gupta@linux.dev>
>
> bpf: keep the range across a sign extension that cannot change it
>
> This commit improves the verifier's handling of sign extensions in
> coerce_reg_to_size_sx() by recognizing when a register's bounds already fit
> within the target field. This prevents unnecessary precision loss when a value
> range straddles zero, allowing valid programs to pass verification.
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 58e788f53ae58..eb093194e2a36 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>
>> @@ -5773,6 +5774,27 @@ static void coerce_reg_to_size_sx(struct bpf_reg_state *reg, int size)
>>   		return;
>>   	}
>>   
>> +	if (size == 1) {
>> +		field_smin = S8_MIN;
>> +		field_smax = S8_MAX;
>> +	} else if (size == 2) {
>> +		field_smin = S16_MIN;
>> +		field_smax = S16_MAX;
>> +	} else {
>> +		/* size == 4 */
>> +		field_smin = S32_MIN;
>> +		field_smax = S32_MAX;
>> +	}
>> +
>> +	/*
>> +	 * The range already fits the field, so (sN)v == v for every value the
>> +	 * register can hold and the sign extension changes nothing. The tests
>> +	 * below cannot reach this case once smin is negative: a negative smin
>> +	 * and a non-negative smax never share their high bits.
>> +	 */
>> +	if (reg_smin(reg) >= field_smin && reg_smax(reg) <= field_smax)
>> +		return;
>> +
>>   	top_smax_value = ((u64)reg_smax(reg) >> num_bits) << num_bits;
>>   	top_smin_value = ((u64)reg_smin(reg) >> num_bits) << num_bits;
> [Severity: Medium]
> Does the 32-bit counterpart function coerce_subreg_to_size_sx() need the same
> fix?
>
> It seems to retain the same precision loss flaw when handling 32-bit sign
> extensions on ranges that straddle zero (e.g., [-4095, 0]). Valid BPF programs
> using 32-bit MOVSX on such ranges will needlessly lose bounds precision and
> might be incorrectly rejected by the verifier.

Good catch. Fixed now. It would only need to handle sizes 1 and 2 though.


> [Severity: High]
> This is a pre-existing issue, but I noticed a potential out-of-bounds array
> read in the BPF memory cgroup kfuncs.
>
> Enums are treated as scalars by the verifier and can be negative. The bounds
> checks in these functions use single-sided signed comparisons, which bypass
> negative values:
>
> mm/bpf_memcontrol.c:bpf_mem_cgroup_memory_events() {

True signature has enum type

   __bpf_kfunc unsigned long bpf_mem_cgroup_memory_events(struct 
mem_cgroup *memcg,
                                                          enum 
memcg_memory_event event)

>      ...
> 	if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))
> 		return (unsigned long)-1;
>
> 	return atomic_long_read(&memcg->memory_events[event]);
> }
>
> If a BPF program passes -1, the check evaluates to false, leading to an
> out-of-bounds read on memcg->memory_events[-1].

Not an issue. enum doesn't have any negative values and with enum tye in 
signature above it will treated as unsigned int so -1 will come out as 
large number not -1.

> A similar issue exists in bpf_mem_cgroup_vm_events(), where the bounds check
> delegates to memcg_vm_event_item_valid():
>
> mm/memcontrol.c:memcg_vm_event_item_valid() {
>      ...
> 	if (idx >= NR_VM_EVENT_ITEMS)
> 		return false;
>      ...
> }
>
> This also performs a single-sided check and fails to catch negative indices.
> Should these be updated to explicitly check for negative values as well?

Same as above since this enum doesn't have negative entries either.

Thx,
-Vineet