Re: [PATCH bpf-next v4 01/15] bpf: Read a kfunc's __sz argument only when it is in a register

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing negative bounds check for enum parameters in memcg kfuncs allows out-of-bounds array read
--

commit 5816e44e0d469f2be29753390131ba5c9f8fd073
Author: Yonghong Song <yonghong.song@linux.dev>

bpf: Read a kfunc's __sz argument only when it is in a register

This commit fixes an issue where an out-of-bounds index could be used to read
from const_reg_vals[] when a kfunc receives a stack argument pair of a
pointer and size. It ensures size_reg is bounded by MAX_BPF_FUNC_REG_ARGS
to prevent reading unrelated callee-saved registers.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 617a277c3558c..0c6a404167ca4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13641,13 +13641,14 @@ s64 bpf_kfunc_stack_access_bytes(struct bpf_verifier_env *env, struct bpf_insn *
>  		goto out;
>  	}
>  
> -	/* ptr + __sz/__szk pair: size is in the next register */
> +	/* ptr + __sz/__szk pair: the size follows the pointer */
>  	if (arg + 1 < nargs &&
>  	    (btf_param_match_suffix(btf, &args[arg + 1], "__sz") ||
>  	     btf_param_match_suffix(btf, &args[arg + 1], "__szk"))) {
>  		int size_reg = BPF_REG_1 + arg + 1;
>  
> -		if (aux->const_reg_mask & BIT(size_reg)) {
> +		if (size_reg <= MAX_BPF_FUNC_REG_ARGS &&
> +		    (aux->const_reg_mask & BIT(size_reg))) {
>  			size = (s64)aux->const_reg_vals[size_reg];
>  			goto out;
>  		}

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with how the
verifier handles enum arguments for kfuncs, specifically in the memcg
kfuncs.

Since the BPF verifier treats enum types as plain signed integers and does not
enforce enum ranges natively, BPF programs can pass negative scalar values
to kfuncs.

In 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]);

Does this signed comparison without a negative bound allow a negative
event value to bypass the check and cause an out-of-bounds read on the
memory_events array?

Similarly, in mm/bpf_memcontrol.c:bpf_mem_cgroup_vm_events():

	if (unlikely(!memcg_vm_event_item_valid(event)))
		return (unsigned long)-1;

	return memcg_events(memcg, event);

And the check in mm/memcontrol.c:memcg_vm_event_item_valid():

	if (idx >= NR_VM_EVENT_ITEMS)
		return false;

	return !BAD_STAT_IDX(memcg_events_index(idx));

Could a negative idx here bypass the bounds check and lead to an
out-of-bounds access on the mem_cgroup_events_index array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912195156.980886-1-yonghong.song@linux.dev?part=1