Re: [PATCH] mm/bpf_memcontrol: mark BPF memcg kfuncs static

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The kfunc `bpf_mem_cgroup_memory_events` lacks a negative bounds check for its enum parameter, allowing an out-of-bounds array read.
- [High] The kfunc `bpf_mem_cgroup_vm_events` uses a signed enum parameter without a negative bounds check, leading to an out-of-bounds array read.
--

commit 69ebb23c6ec72d0bfb8ea52e71248a3737fbb2f4
Author: JP Kobryn <jp.kobryn@linux.dev>

mm/bpf_memcontrol: mark BPF memcg kfuncs static

The patch marks kfuncs in bpf_memcontrol.c as static since they are only
referenced by BPF programs and resolved through BTF. This removes unnecessary
external linkage for these functions.

Closes: https://lore.kernel.org/oe-kbuild-all/202606150332.Xy4Egd9s-lkp@intel.com/

> diff --git a/mm/bpf_memcontrol.c b/mm/bpf_memcontrol.c
> index 716df49d76477..eff79bb2c7581 100644
> --- a/mm/bpf_memcontrol.c
> +++ b/mm/bpf_memcontrol.c

[ ... ]

> @@ -89,8 +89,8 @@ __bpf_kfunc void bpf_put_mem_cgroup(struct mem_cgroup *memcg)
>   *
>   * Return: The current value of the corresponding events counter.
>   */
> -__bpf_kfunc unsigned long bpf_mem_cgroup_vm_events(struct mem_cgroup *memcg,
> -						   enum vm_event_item event)
> +__bpf_kfunc static unsigned long
> +bpf_mem_cgroup_vm_events(struct mem_cgroup *memcg, enum vm_event_item event)
>  {
>  	if (unlikely(!memcg_vm_event_item_valid(event)))

[Severity: High]
This is a pre-existing issue, but does memcg_vm_event_item_valid()
adequately check for negative values?

Since the BPF verifier does not validate enum ranges, the event argument
could be negative. Looking at memcg_vm_event_item_valid() in
mm/memcontrol.c, it checks:

    if (idx >= NR_VM_EVENT_ITEMS)
        return false;

Because both operands are signed, negative values bypass the check.

Can this lead to an out-of-bounds read when the negative index is
eventually passed to memcg_events(memcg, event)?

>  		return (unsigned long)-1;

[ ... ]

> @@ -122,8 +122,8 @@ __bpf_kfunc unsigned long bpf_mem_cgroup_usage(struct mem_cgroup *memcg)
>   *
>   * Return: The current value of the memory event counter.
>   */
> -__bpf_kfunc unsigned long bpf_mem_cgroup_memory_events(struct mem_cgroup *memcg,
> -						       enum memcg_memory_event event)
> +__bpf_kfunc static unsigned long
> +bpf_mem_cgroup_memory_events(struct mem_cgroup *memcg, enum memcg_memory_event event)
>  {
>  	if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))

[Severity: High]
This isn't a bug introduced by this patch, but does this bounds check
handle negative values properly?

Because the event parameter is a signed enum and the BPF verifier allows
arbitrary integers for enum arguments, a negative value bypasses the
event >= MEMCG_NR_MEMORY_EVENTS check.

Can this result in an out-of-bounds read when accessing
&memcg->memory_events[event] right below?

>  		return (unsigned long)-1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617202147.78347-1-jp.kobryn@linux.dev?part=1