Re: [PATCH bpf-next v3] selftests/bpf: Add tests for memcg kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: Missing negative bounds check in enum memcg kfuncs allows OOB read
--

commit 555191733d8248d3b4bb6e02814568e60e2be094
Author: Brahmajit Das <bdas@suse.de>

selftests/bpf: Add tests for memcg kfuncs

The existing cgroup_iter_memcg test covers bpf_get_mem_cgroup(),
bpf_put_mem_cgroup(), bpf_mem_cgroup_flush_stats(),
bpf_mem_cgroup_page_state() and bpf_mem_cgroup_vm_events(), but
bpf_get_root_mem_cgroup(), bpf_mem_cgroup_usage(),
bpf_mem_cgroup_memory_events() and bpf_proactive_reclaim() have no
selftest coverage.

> diff --git a/tools/testing/selftests/bpf/progs/memcg_kfuncs.c b/tools/testing/selftests/bpf/progs/memcg_kfuncs.c
> new file mode 100644
> index 0000000000000..5e2ea19a072da
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/memcg_kfuncs.c
[ ... ]
> @@ -0,0 +1,106 @@
[ ... ]
> +SEC("syscall")
> +int memcg_query(void *ctx)
> +{
[ ... ]
> +	/* Out-of-range indices must be rejected with (unsigned long)-1. */
> +	bad_memory_event = bpf_mem_cgroup_memory_events(memcg,
> +		bpf_core_enum_value(enum memcg_memory_event, MEMCG_NR_MEMORY_EVENTS));
> +	bad_vm_event = bpf_mem_cgroup_vm_events(memcg,
> +		bpf_core_enum_value(enum vm_event_item, NR_VM_EVENT_ITEMS));
> +	bad_page_state = bpf_mem_cgroup_page_state(memcg, -1);
> +	/* Negative indices must not bypass the bounds checks. */
> +	neg_memory_event = bpf_mem_cgroup_memory_events(memcg, -1);
> +	neg_vm_event = bpf_mem_cgroup_vm_events(memcg, -1);

[Severity: High]
Does this test inadvertently trigger an out-of-bounds read in the kernel?

Looking at bpf_mem_cgroup_memory_events() in mm/bpf_memcontrol.c, the enum
memcg_memory_event parameter is treated as a signed integer, and the bounds
check uses a signed comparison:

	if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))
		return (unsigned long)-1;

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

Since the BPF verifier does not check enum ranges, passing a negative value
like -1 from this test bypasses the >= check entirely and accesses the
memory_events array out of bounds.

Similarly, memcg_vm_event_item_valid() in mm/memcontrol.c, which is used by
bpf_mem_cgroup_vm_events(), also uses a signed comparison:

	if (idx >= NR_VM_EVENT_ITEMS)
		return false;

	return !BAD_STAT_IDX(memcg_events_index(idx));

This allows the negative index passed by this test to bypass the check and
cause an out-of-bounds access in memcg_events_index().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013171211.1661726-1-listout@listout.xyz?part=1