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

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

[Pu Lehui]

From: Pu Lehui <pulehui@huawei.com>

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 alloc, assign and link the storage for the new prog
during link_update.

Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active bpf_cgroup_link")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 kernel/bpf/cgroup.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 4355ccb78a9c..52b5b685a93c 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1014,6 +1014,7 @@ static void replace_effective_prog(struct cgroup *cgrp,
 				desc->bpf.effective[atype],
 				lockdep_is_held(&cgroup_mutex));
 		item = &progs->items[pos];
+		bpf_cgroup_storages_assign(item->cgroup_storage, pl->storage);
 		WRITE_ONCE(item->prog, pl->link->link.prog);
 	}
 }
@@ -1032,6 +1033,8 @@ 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 *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
 	enum cgroup_bpf_attach_type atype;
 	struct bpf_prog *old_prog;
 	struct bpf_prog_list *pl;
@@ -1056,10 +1059,16 @@ 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;
+
 	cgrp->bpf.revisions[atype] += 1;
 	old_prog = xchg(&link->link.prog, new_prog);
+	bpf_cgroup_storages_assign(pl->storage, storage);
 	replace_effective_prog(cgrp, atype, pl);
 	bpf_prog_put(old_prog);
+	bpf_cgroup_storages_link(new_storage, cgrp, link->link.attach_type);
 	return 0;
 }
 
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] In-place updates of `item->cgroup_storage` in the active RCU-protected `bpf_prog_array` cause data races with concurrent RCU readers, leading to NULL pointer dereferences or Out-Of-Bounds memory accesses.
--

--- Patch [1]: [PATCH bpf] bpf, cgroup: Fix storage null-ptr-deref after replacing prog ---
commit 3d155d588030a7a30403e9ee7737c108f47b5cb3
Author: Pu Lehui <pulehui@huawei.com>

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

This commit fixes a storage null pointer dereference when replacing a
program by allocating, assigning, and linking the required cgroup storage
for the replacement program during link_update.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 4355ccb78a9c6..52b5b685a93c5 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -1014,6 +1014,7 @@ static void replace_effective_prog(struct cgroup *cgrp,
>  				desc->bpf.effective[atype],
>  				lockdep_is_held(&cgroup_mutex));
>  		item = &progs->items[pos];
> +		bpf_cgroup_storages_assign(item->cgroup_storage, pl->storage);

[Severity: High]
Does this in-place update of item->cgroup_storage create a data race with
concurrent RCU readers?

If CPU1 is executing bpf_prog_run_array_cg() under the RCU read lock and
starts running the old program:

CPU1:
bpf_prog_run_array_cg()
    while ((prog = READ_ONCE(item->prog))) {
        run_ctx.prog_item = item;
        func_ret = run_prog(prog, ctx);

And CPU2 concurrently replaces the program with one that doesn't use storage,
it updates the storage in-place:

CPU2:
replace_effective_prog()
    bpf_cgroup_storages_assign(item->cgroup_storage, pl->storage);
    WRITE_ONCE(item->prog, pl->link->link.prog);

If CPU1's executing program then calls bpf_get_local_storage(), it could
read the updated NULL storage pointer while still executing the old program:

CPU1:
bpf_get_local_storage()
    storage = ctx->prog_item->cgroup_storage[stype];
    if (stype == BPF_CGROUP_STORAGE_SHARED)
        ptr = &READ_ONCE(storage->buf)->data[0];

Could this lead to a NULL pointer dereference or out-of-bounds access?

>  		WRITE_ONCE(item->prog, pl->link->link.prog);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714014659.401063-1-pulehui@huaweicloud.com?part=1