Re: [PATCH bpf-next v2 05/23] bpf: Clarify unused and scalar function argument types

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds array access in memcg kfuncs (`bpf_mem_cgroup_memory_events` and `bpf_mem_cgroup_vm_events`) due to missing negative bounds checks for enum arguments.
--

commit a779fb1811c9555981f50c14118ca53a762cdd76
Author: Amery Hung <ameryhung@gmail.com>

bpf: Clarify unused and scalar function argument types

This commit renames ARG_DONTCARE to ARG_UNUSED to indicate it terminates
argument iteration. It also reuses ARG_SCALAR for integer, enum, and 
exception callback arguments to reflect that they require SCALAR_VALUE.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 42d89db5df07b..5d21bee458055 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -9818,7 +9815,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
>  		struct bpf_reg_state *reg = get_func_arg_reg(caller, regs, i);
>  		struct bpf_subprog_arg_info *arg = &sub->args[i];
>  
> -		if (arg->arg_type == ARG_ANYTHING) {
> +		if (arg->arg_type == ARG_SCALAR) {
>  			if (reg->type != SCALAR_VALUE) {
>  				bpf_log(log, "%s is not a scalar\n", reg_arg_name(env, argno));
>  				return -EINVAL;

[Severity: High]
This is a pre-existing issue, but while reviewing how the verifier treats
enum arguments as SCALAR_VALUE, I noticed that the memcg kfuncs might be
vulnerable to negative enum values.

Because the BPF verifier treats enum arguments as scalars without enforcing
their ranges, a BPF program can pass a negative value (like -1). 

In bpf_mem_cgroup_memory_events(), the bounds check uses a signed
comparison:

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 event is negative, does this code overflow the memory_events[] array
and allow an out-of-bounds read?

Similarly, in bpf_mem_cgroup_vm_events(), it relies on
memcg_vm_event_item_valid() for bounds checking:

mm/bpf_memcontrol.c:bpf_mem_cgroup_vm_events() {
	if (unlikely(!memcg_vm_event_item_valid(event)))
		return (unsigned long)-1;
}

mm/memcontrol.c:memcg_vm_event_item_valid() {
	if (idx >= NR_VM_EVENT_ITEMS)
		return false;

	return !BAD_STAT_IDX(memcg_events_index(idx));
}

If idx is negative, will this bypass the >= NR_VM_EVENT_ITEMS check and
cause an out-of-bounds access in memcg_events_index()? 

Should these kfuncs be updated to explicitly check for negative values
(e.g., event < 0 || event >= MEMCG_NR_MEMORY_EVENTS)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911220415.1396439-1-ameryhung@gmail.com?part=5