Re: [PATCH bpf-next v8 1/2] mm/bpf: Add bpf_proactive_reclaim kfunc

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The kfuncs `bpf_mem_cgroup_memory_events` and `bpf_mem_cgroup_vm_events` fail to properly bounds-check their enum `event` parameters for negative values, leading to out-of-bounds array reads.
--

commit 43c46156245f91f265307f6d4888bcb12e5294d0
Author: Hui Zhu <zhuhui@kylinos.cn>

mm/bpf: Add bpf_proactive_reclaim kfunc

Add bpf_proactive_reclaim(), a sleepable kfunc which performs one
proactive reclaim pass on a given memory cgroup, similar to a write
to memory.reclaim but without retrying until the target is reached.

> diff --git a/mm/bpf_memcontrol.c b/mm/bpf_memcontrol.c
> index 716df49d76477..74ccac30c0117 100644
> --- a/mm/bpf_memcontrol.c
> +++ b/mm/bpf_memcontrol.c
[ ... ]
> @@ -171,22 +239,44 @@ BTF_ID_FLAGS(func, bpf_mem_cgroup_memory_events)

[Severity: High]
This isn't a bug introduced by this patch, but does this function fail to
validate negative values passed to the event parameter?

The BPF verifier does not validate that enum parameters remain within valid
constant ranges and allows negative scalar values since they are treated as
integers. 

Because enums are signed integers in C by default, negative values can bypass
the bounds check using 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 a BPF program passes a negative event value, could it bypass the >= check
and trigger an out-of-bounds read in the memory_events array?

This same issue seems to apply to bpf_mem_cgroup_vm_events() which calls
memcg_vm_event_item_valid():

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

Which is then used as an array index:

mm/memcontrol.c:memcg_events_index() {
	return mem_cgroup_events_index[idx];
}

Could negative values here also lead to out-of-bounds memory reads, potentially
resulting in information leaks or kernel panics?

>  BTF_ID_FLAGS(func, bpf_mem_cgroup_usage)
>  BTF_ID_FLAGS(func, bpf_mem_cgroup_page_state)
>  BTF_ID_FLAGS(func, bpf_mem_cgroup_flush_stats, KF_SLEEPABLE)
> -
>  BTF_KFUNCS_END(bpf_memcontrol_kfuncs)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788764121.git.zhuhui@kylinos.cn?part=1