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

On 2026/7/22 15:48, Pu Lehui wrote:
> 
> On 2026/7/22 15:35, [email protected] wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential 
>> issue(s) to consider:
>>
>> Pre-existing issues:
>> - [High] Missing rollback of `pl->flags` on the error path in 
>> `__cgroup_bpf_attach` permanently corrupts the execution order of the 
>> replaced program.
>> -- 
>>
>> --- Patch [4]: [PATCH bpf v6 4/4] bpf, cgroup: Fix storage 
>> null-ptr-deref after replacing prog ---
>> commit f8726e124d0a4e72b87976830f0741e286d474df
>> Author: Pu Lehui <[email protected]>
>>
>> bpf, cgroup: Fix storage null-ptr-deref after replacing prog
>>
>> Syzkaller reported a storage null-ptr-deref issue after replacing a prog.
>> The issue is fixed by properly allocating the storage and comparing 
>> the old
>> and new storage pointers, falling back to an RCU-safe update if they 
>> change.
>> Additionally, proper rollbacks for storage and flags are added to the 
>> error
>> path in __cgroup_bpf_attach for code rigor.
>>
>>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>>> --- a/kernel/bpf/cgroup.c
>>> +++ b/kernel/bpf/cgroup.c
>>> @@ -883,7 +885,10 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>>>       pl->prog = prog;
>>>       pl->link = link;
>>>       pl->flags = flags;
>>> +    if (old_prog)
>>> +        bpf_cgroup_storages_assign(old_storage, pl->storage);
>>>       bpf_cgroup_storages_assign(pl->storage, storage);
>>> +    old_flags = cgrp->bpf.flags[atype];
>>>       cgrp->bpf.flags[atype] = saved_flags;
>>>       if (type == BPF_LSM_CGROUP) {
>>
>> [ ... ]
>>
>>> @@ -915,12 +920,14 @@ static int __cgroup_bpf_attach(struct cgroup 
>>> *cgrp,
>>>       if (old_prog) {
>>>           pl->prog = old_prog;
>>>           pl->link = NULL;
>>> +        bpf_cgroup_storages_assign(pl->storage, old_storage);
>>>       }
>>
>> [Severity: High]
>> This is a pre-existing issue, but since this patch tightens the error 
>> handling
>> and rollbacks in __cgroup_bpf_attach(), should we also restore the 
>> original
>> pl->flags here?
> 
> 
>>
>> Earlier in the function, pl->flags is overwritten with the incoming 
>> flags:
>>
>>     pl->prog = prog;
>>     pl->link = link;
>>     pl->flags = flags;
>>
>> If update_effective_progs() later fails (e.g., due to memory allocation

update_effective_progs() is unlikely to fail, whereas 
bpf_trampoline_link_cgroup_shim() can fail.

In __cgroup_bpf_attach(), when replacing an existing bpf_link in a 
single-attach scenario, old_prog is NULL while old_link holds the active 
link. If attachment fails mid-way during 
bpf_trampoline_link_cgroup_shim() or update_effective_progs(), the 
cleanup path checks if (!old_prog). This incorrectly identifies the 
existing pl entry as newly allocated and wrongly frees it via kfree(), 
triggering a Use-After-Free (UAF) bug.

To resolve this cleanly, I will split this into two individual patches.

> 
> It's hard to make update_effective_progs fail in __cgroup_bpf_attach, as 
> it will failed only when allocating failed:
> 
> progs = bpf_prog_array_alloc(cnt, GFP_KERNEL);
> 
> failed for the above alloc is not a high issue, as cnt max is no more 
> than BPF_CGROUP_MAX_PROGS.
> 
>> failure), the error path restores pl->prog, pl->link, and pl->storage, 
>> but
>> misses pl->flags. If a user attempts to replace a program using 
>> BPF_F_REPLACE
>> with different BPF_F_PREORDER settings and the replacement fails, 
>> could this
>> leave the original program active but with its execution order silently
>> corrupted?
>>
>>>       bpf_cgroup_storages_free(new_storage);
>>>       if (!old_prog) {
>>>           hlist_del(&pl->node);
>>>           kfree(pl);
>>>       }
>>> +    cgrp->bpf.flags[atype] = old_flags;
>>>       return err;
>>>   }
>>