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

[Pu Lehui]

On 2026/8/5 6:56, Andrii Nakryiko wrote:
> On Tue, Jul 28, 2026 at 6:26 AM Pu Lehui <pulehui@xxxxxxxxxxxxxxx> wrote:
> > From: Pu Lehui <pulehui@xxxxxxxxxx>
> >
> > 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 <ameryhung@xxxxxxxxx>
> > Signed-off-by: Pu Lehui <pulehui@xxxxxxxxxx>
> > ---
> > 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/20260728132336.2857800-1-pulehui@xxxxxxxxxxxxxxx [0]
> > Link: https://lore.kernel.org/bpf/f87b53c0-8f00-45a6-82db-8242fa9b143f@xxxxxxxxxxxxxxx [1]
> >
> > v3: https://lore.kernel.org/bpf/20260720134547.1289964-5-pulehui@xxxxxxxxxxxxxxx
> > - 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/20260717073343.958862-1-pulehui@xxxxxxxxxxxxxxx
> > - Fix invalid access for in-place update when storage changed. (Sashiko)
> >
> > v1: https://lore.kernel.org/bpf/20260714014659.401063-1-pulehui@xxxxxxxxxxxxxxx
> >
> >   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