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

On 2026/8/5 6:56, Andrii Nakryiko wrote:
> On Tue, Jul 28, 2026 at 6:26 AM Pu Lehui <[email protected]> wrote:
>>
>> From: Pu Lehui <[email protected]>
>>
>> Syzkaller reported a storage null-ptr-deref issue after replacing prog.
>> This occurs in the following scenario:
>> 1. prog A, an empty prog, is attached to a cgrp.
>> 2. prog B uses BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE and calls the
>>     bpf_get_local_storage helper.
>> 3. link_update is called to replace prog A with prog B.
>>
>> The reason is that __cgroup_bpf_replace fails to alloc and assign the
>> required cgrp storage for the incoming replacement prog. Consequently,
>> the new prog inherits an uninit storage, leading to null-ptr-deref panic
>> when kick the new prog.
>>
>> Fix this by properly allocating the storage and comparing the old and
>> new storage pointers. If the storage changed, fallback to
>> update_effective_progs which performs a RCU-safe update of the entire
>> array. If the storage remains unchanged, we can safely retain the
>> fast-path in-place update.
>>
>> Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active bpf_cgroup_link")
>> Reviewed-by: Amery Hung <[email protected]>
>> Signed-off-by: Pu Lehui <[email protected]>
>> ---
>> v4:
>> - Extract the fix for __cgroup_bpf_attach() into a standalone patch [0].
>> - Add Reviewed-by tag by Amery.
>> - Separate from patchset [1]. (Andrii)
>>
>> Link: https://lore.kernel.org/bpf/[email protected] [0]
>> Link: https://lore.kernel.org/bpf/[email protected] [1]
>>
>> v3: https://lore.kernel.org/bpf/[email protected]
>> - Include the storage and flags rollbacks to patch4 for sake of code rigor,
>>    as it's hard to make update_effective_progs fail in __cgroup_bpf_attach.
>>
>> v2: https://lore.kernel.org/bpf/[email protected]
>> - Fix invalid access for in-place update when storage changed. (Sashiko)
>>
>> v1: https://lore.kernel.org/bpf/[email protected]
>>
>>   kernel/bpf/cgroup.c | 37 ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 4355ccb78a9c..e2fa0ebeed83 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -1032,11 +1032,17 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
>>                                  struct bpf_cgroup_link *link,
>>                                  struct bpf_prog *new_prog)
>>   {
>> +       struct bpf_cgroup_storage *new_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
>> +       struct bpf_cgroup_storage *old_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
>> +       struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
>> +       enum bpf_cgroup_storage_type stype;
>>          enum cgroup_bpf_attach_type atype;
>> +       bool storage_changed = false;
>>          struct bpf_prog *old_prog;
>>          struct bpf_prog_list *pl;
>>          struct hlist_head *progs;
>>          bool found = false;
>> +       int err;
>>
>>          atype = bpf_cgroup_atype_find(link->link.attach_type, new_prog->aux->attach_btf_id);
>>          if (atype < 0)
>> @@ -1056,10 +1062,39 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
>>          if (!found)
>>                  return -ENOENT;
>>
>> +       if (bpf_cgroup_storages_alloc(storage, new_storage, link->link.attach_type,
>> +                                     new_prog, cgrp))
>> +               return -ENOMEM;
>> +
>> +       for_each_cgroup_storage_type(stype) {
>> +               if (storage[stype] != pl->storage[stype]) {
>> +                       storage_changed = true;
>> +                       break;
>> +               }
>> +       }
>> +
>>          cgrp->bpf.revisions[atype] += 1;
>>          old_prog = xchg(&link->link.prog, new_prog);
>> -       replace_effective_prog(cgrp, atype, pl);
>> +
>> +       if (!storage_changed) {
>> +               replace_effective_prog(cgrp, atype, pl);
>> +               bpf_prog_put(old_prog);
>> +               return 0;
>> +       }
>> +
>> +       bpf_cgroup_storages_assign(old_storage, pl->storage);
>> +       bpf_cgroup_storages_assign(pl->storage, storage);
>> +       err = update_effective_progs(cgrp, atype);
>> +       if (err) {
>> +               xchg(&link->link.prog, old_prog);
> 
> I'm not a fan of having this transient link.prog transition from old
> to new and back to old prog if something goes wrong.
> 
> Given this is syzbot (only) reported issue, how about we just check
> that cgroup storage (legacy one at that) of old and new program is
> compatible, and if not, just reject link update? that would be less
> risky change

Ah, I didn't think of that! It seems to be much simpler.

> 
> pw-bot: cr
> 
> 
>> +               bpf_cgroup_storages_assign(pl->storage, old_storage);
>> +               bpf_cgroup_storages_free(new_storage);
>> +               cgrp->bpf.revisions[atype] -= 1;
>> +               return err;
>> +       }
>> +
>>          bpf_prog_put(old_prog);
>> +       bpf_cgroup_storages_link(new_storage, cgrp, link->link.attach_type);
>>          return 0;
>>   }
>>
>> --
>> 2.34.1
>>