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

[sashiko-bot@kernel.org]

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.

[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() {
    ...
	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].

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910164635.459558-1-vineet.gupta@linux.dev?part=5