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

On 2026/8/5 12:05, [email protected] wrote:
> 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?

Oops, sorry about that. I forgot to update commit message as I thought 
-EINVAL seems more appropriate here.

I'll wait for the review feedback first. If the patch looks good 
overall, I'd be very grateful if the maintainer could fix up the commit 
message on apply. Otherwise, I'm happy to resend.


> 
>> 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.

Fixed.

https://git.kernel.org/bpf/bpf-next/c/6655c409707e

> 
> 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()?
> 

Fixed.

https://git.kernel.org/bpf/bpf-next/c/863f3ddd0b8a