Re: [PATCH bpf v4 1/2] bpf: Fix queue/stack map u32 index overflow

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

Pre-existing issues:
- [High] Kfuncs using enum parameters as array indices lack bounds checking for negative values.
--

--- Patch [1]: [PATCH bpf v4 1/2] bpf: Fix queue/stack map u32 index overflow ---
commit b3cc6df8f8f6a83f377bada50a4de0c2ebde4779
Author: Yuan Chen <[email protected]>

bpf: Fix queue/stack map u32 index overflow

This commit fixes an integer overflow in queue/stack maps by restoring a
bounds check in queue_stack_map_alloc_check(). It rejects max_entries and
value_size combinations that would reach or exceed U32_MAX bytes.

[ ... ]

While reviewing BPF bounds checking, I noticed a pre-existing issue in
mm/bpf_memcontrol.c:

> mm/bpf_memcontrol.c
> 
> __bpf_kfunc unsigned long bpf_mem_cgroup_vm_events(struct mem_cgroup *memcg,
> 						   enum vm_event_item event)
> {
> 	if (unlikely(!memcg_vm_event_item_valid(event)))
> 		return (unsigned long)-1;

[Severity: High]
This is a pre-existing issue, but since enums are represented as signed
integers in C, could a BPF program bypass this bounds check by passing a
negative enum value?

It appears that a negative event value could evade the upper bounds check
and lead to an out-of-bounds read when the event is used as an array index.

> 
> 	return memcg_events(memcg, event);
> }
> 
> ...
> 
> __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;

[Severity: High]
This isn't a bug introduced by this patch, but does this signed comparison
handle negative event values?

Similar to the above, a negative event value would pass this check and
subsequently be used as a negative index for the memory_events array.
Does this result in an out-of-bounds memory read?

> 
> 	return atomic_long_read(&memcg->memory_events[event]);
> }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1