Re: [PATCH bpf v5] bpf, cgroup: Fix storage null-ptr-deref after replacing prog

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

New issues:
- [Medium] The commit message claims to reject the link update with -EPERM, but the code actually returns -EINVAL.

Pre-existing issues:
- [High] pl->storage is not rolled back in the error path of __cgroup_bpf_attach(), leading to a Use-After-Free of cgroup storages.
- [High] Lockless access to `link->prog` in `bpf_link_show_fdinfo` and `bpf_link_get_info_by_fd` can lead to a Use-After-Free.
--

--- Patch [1]: [PATCH bpf v5] bpf, cgroup: Fix storage null-ptr-deref after replacing prog ---
commit f5ba6e7a5022bb7261a93a1c9312a6415db0249f
Author: Pu Lehui <[email protected]>

bpf, cgroup: Fix storage null-ptr-deref after replacing prog

This commit fixes a null pointer dereference issue when replacing a BPF
program attached to a cgroup. It addresses the problem by rejecting link
updates if the new program's cgroup storage is incompatible with the existing
link's program.

Link: https://lore.kernel.org/bpf/[email protected]
Link: https://lore.kernel.org/bpf/[email protected]

>     Fix this by rejecting link update with -EPERM if new_prog's cgroup
>     storage is incompatible with link->prog.

[Severity: Medium]
The commit message claims to reject the link update with -EPERM, but the new
code actually returns -EINVAL. Could the commit message be updated to match
the implementation?

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

[ ... ]

> @@ -1056,6 +1070,9 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
>  	if (!found)
>  		return -ENOENT;
>  
> +	if (!cgroup_bpf_storages_compatible(link->link.prog, new_prog))
> +		return -EINVAL;
> +
>  	cgrp->bpf.revisions[atype] += 1;
>  	old_prog = xchg(&link->link.prog, new_prog);
>  	replace_effective_prog(cgrp, atype, pl);

[Severity: High]
This is a pre-existing issue, but while reviewing this code I noticed a
potential use-after-free in the error path of __cgroup_bpf_attach().

In __cgroup_bpf_attach(), bpf_cgroup_storages_assign(pl->storage, storage)
updates the storage pointers in the existing program list element pl to point
to the storages of the new program. 

If a subsequent operation like update_effective_progs() fails, the code jumps
to the cleanup label:

kernel/bpf/cgroup.c:__cgroup_bpf_attach() {
    ...
cleanup:
	if (old_prog) {
		pl->prog = old_prog;
		pl->link = NULL;
	}
	bpf_cgroup_storages_free(new_storage);
    ...
}

In this cleanup block, pl->prog is reverted to old_prog, but pl->storage is
never reverted to its original state. Any newly allocated storages for the
failed program, which reside in new_storage, are freed via
bpf_cgroup_storages_free(new_storage).

Because pl remains in the cgroup's active program list, its storage array now
contains dangling pointers to these freed RCU-reclaimed storages. Can any
future execution of the BPF program dereference them, resulting in a
use-after-free?


[Severity: High]
This is a pre-existing issue, but I also noticed potential lockless access to
link->prog which could lead to a use-after-free.

BPF link types that support dynamic program updates (such as cgroup links)
replace their attached program by atomically swapping link->prog (using xchg)
and then calling bpf_prog_put() on the old program. This decreases the
reference count and defers the actual freeing of the program until after an
RCU grace period.

However, bpf_link_show_fdinfo() fetches link->prog locklessly and
dereferences its tag without holding rcu_read_lock():

kernel/bpf/syscall.c:bpf_link_show_fdinfo() {
    ...
	const struct bpf_prog *prog = link->prog;
    ...
	if (prog) {
		bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
    ...
}

Similarly, bpf_link_get_info_by_fd() fetches link->prog and accesses its aux
field without RCU protection:

kernel/bpf/syscall.c:bpf_link_get_info_by_fd() {
    ...
	if (link->prog)
		info.prog_id = link->prog->aux->id;
    ...
}

If a program update occurs concurrently and the reader thread is preempted
long enough for the RCU grace period to elapse, would the old program be
freed before the reader dereferences its memory? Should these accesses be
protected by rcu_read_lock()?

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