Re: [PATCH] mm/bpf: Add bpf_mem_cgroup_memory_events_local() kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] mm: out-of-bounds read in bpf_mem_cgroup_memory_events()
  [1]
--

commit 19b0cb61e45dc1f33944548e84f3a699079d3e85
Author: Hongfu Li <lihongfu@kylinos.cn>

mm/bpf: Add bpf_mem_cgroup_memory_events_local() kfunc

This commit adds the bpf_mem_cgroup_memory_events_local() kfunc to allow BPF
programs to read a memory cgroup's local memory event counters.

> diff --git a/mm/bpf_memcontrol.c b/mm/bpf_memcontrol.c
> index d8f579c285600..826f711718dbe 100644
> --- a/mm/bpf_memcontrol.c
> +++ b/mm/bpf_memcontrol.c
> @@ -133,6 +133,23 @@ __bpf_kfunc unsigned long bpf_mem_cgroup_memory_events(struct mem_cgroup *memcg,
>  	return atomic_long_read(&memcg->memory_events[event]);
>  }
>  
> +/**
> + * bpf_mem_cgroup_memory_events_local - Read memory cgroup's local memory event
> + * @memcg: memory cgroup
> + * @event: memory event id
> + *
> + * Return: The current value of the local memory event counter.
> + */
> +__bpf_kfunc unsigned long
> +bpf_mem_cgroup_memory_events_local(struct mem_cgroup *memcg,
> +				   enum memcg_memory_event event)
> +{
> +	if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))
> +		return (unsigned long)-1;
> +
> +	return atomic_long_read(&memcg->memory_events_local[event]);
> +}

[Severity: Critical]
This problem wasn't introduced by this patch, but in the existing
bpf_mem_cgroup_memory_events() right above [1] (and in the new
bpf_mem_cgroup_memory_events_local() here), can a negative event value pass
the bounds check and read out of bounds of memory_events[]?

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

The BPF verifier does not enforce enum bounds on kfunc arguments, so a BPF
program can pass a negative value for event.  If enum memcg_memory_event is a
signed type, event >= MEMCG_NR_MEMORY_EVENTS performs a signed comparison that
evaluates to false for negative numbers, indexing before the start of
memcg->memory_events[] and disclosing adjacent kernel memory or crashing on an
unmapped page.  Checking event < 0 || event >= MEMCG_NR_MEMORY_EVENTS (or
casting event to unsigned int) would reject negative values.

[1] https://sashiko.dev/#/bug/linux-77d600fe-b74d-4e53-9ee2-707d37cb8052

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-for-bpf_memctl-1111-v1-1-8b56784d9e5a@kylinos.cn?part=1