Re: [PATCH bpf v2 1/2] bpf: Fix effective prog array index with BPF_F_PREORDER

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but replace_effective_prog() erroneously uses the logical index returned by effective_prog_pos() as a direct physical index into the effective BPF program array. If previous detach operations failed to reallocate the array and left dummy_bpf_prog elements in place, the physical index does not match the logical index. This causes bpf_link_update() to overwrite the wrong program slot (e.g., the dummy) leaving the original program untouched in the active array while its reference is dropped by __cgroup_bpf_replace(). Once the RCU grace period expires, this leads to a Use-After-Free when the kernel executes the freed program.
--

commit f837441f68b1fdb520696422a141ec421e2d50ae
Author: Amery Hung <ameryhung@gmail.com>

bpf: Fix effective prog array index with BPF_F_PREORDER

This commit introduces a shared helper effective_prog_pos() to accurately
determine a program's position in the effective array by replaying the
placement logic. This fixes an issue where preorder programs caused
replace_effective_prog() and purge_effective_progs() to access incorrect
slots, corrupting the array or causing a use-after-free.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 83ce66296ac19..4355ccb78a9c6 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c

[ ... ]

> @@ -960,27 +1006,15 @@ static void replace_effective_prog(struct cgroup *cgrp,
>  		if (percpu_ref_is_zero(&desc->bpf.refcnt))
>  			continue;
>  
> -		/* find position of link in effective progs array */
> -		for (pos = 0, cg = desc; cg; cg = cgroup_parent(cg)) {
> -			if (pos && !(cg->bpf.flags[atype] & BPF_F_ALLOW_MULTI))
> -				continue;
> -
> -			head = &cg->bpf.progs[atype];
> -			hlist_for_each_entry(pl, head, node) {
> -				if (!prog_list_prog(pl))
> -					continue;
> -				if (pl->link == link)
> -					goto found;
> -				pos++;
> -			}
> -		}
> -found:
> -		BUG_ON(!cg);
> +		pos = effective_prog_pos(desc, atype, pl);
> +		if (WARN_ON_ONCE(pos < 0))
> +			continue;
>  
>  		progs = rcu_dereference_protected(
>  				desc->bpf.effective[atype],
>  				lockdep_is_held(&cgroup_mutex));
>  		item = &progs->items[pos];
> -		WRITE_ONCE(item->prog, link->link.prog);
> +		WRITE_ONCE(item->prog, pl->link->link.prog);

[Severity: Critical]
This isn't a bug introduced by this patch, but does replace_effective_prog()
erroneously use the logical index returned by effective_prog_pos() as a direct
physical index into the effective BPF program array?

If previous detach operations failed to reallocate the array and left
dummy_bpf_prog elements in place, the physical index would not match the
logical index. This could cause bpf_link_update() to overwrite the wrong
program slot (e.g., the dummy), leaving the original program untouched in the
active array while its reference is dropped by __cgroup_bpf_replace().

Once the RCU grace period expires, could this lead to a Use-After-Free when
the kernel executes the freed program? Should this instead use
bpf_prog_array_update_at(progs, pos, pl->link->link.prog) to correctly handle
the presence of dummy programs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619063520.2690547-1-ameryhung@gmail.com?part=1