Re: [PATCH bpf v2] bpf, cgroup: Fix invalid storage access after __cgroup_bpf_attach failed

[Emil Tsalapatis]

On Wed Jul 29, 2026 at 6:02 AM EDT, Pu Lehui wrote:
> From: Pu Lehui <pulehui@xxxxxxxxxx>
>
> Sashiko reported a potential invalid storage access issue after
> replacing a cgroup bpf prog.
>

Nit: No need to mention Sashiko reports in the commit message imo,
the Reported-by is enough and imo it makes it confusing for people
not familiar with the system.

> This occurs in the following scenario:
> 1. prog1 with storage is attached to a cgroup in multi-attach mode.
> 2. prog1 is replaced with prog2 using BPF_F_REPLACE in multi-attach
>    mode, but fails midway (e.g. in bpf_trampoline_link_cgroup_shim or
>    update_effective_progs).
> 3. A new prog3 is attached to the cgroup in multi-attach mode.
>
> The reason is that __cgroup_bpf_attach overwrites pl->storage with the
> new storage prior to attachment completion. When attachment fails
> midway, the cleanup path calls bpf_cgroup_storages_free(new_storage) to
> free the newly allocated storage, but fails to restore pl->storage back
> to old_storage.
>
> Consequently, the still-active prog1 holds invalid or dangling storage
> pointers, leading to an invalid memory access when prog1 executes and
> calls bpf_get_local_storage. Additionally, original pl->flags and
> cgrp->bpf.flags[atype] are left unrestored.
>
> Fix this by saving old_pl_flags, old_storage, and old_flags prior to the
> update, and properly restoring all of them in the cleanup path on error.
>
> Fixes: 7d9c3427894f ("bpf: Make cgroup storages shared between programs on the same cgroup")
> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> Signed-off-by: Pu Lehui <pulehui@xxxxxxxxxx>

Reviewed-by: Emil Tsalapatis <emil@xxxxxxxxxxxxxxx>

> ---
> v2:
> - Remove the link relative code as link only support
>   BPF_F_ALLOW_MULTI attach, so will not occur UAF pl.
>
> v1: https://lore.kernel.org/bpf/20260728132336.2857800-1-pulehui@xxxxxxxxxxxxxxx
>
>  kernel/bpf/cgroup.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index e2fa0ebeed83..be24ca453cab 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -813,8 +813,10 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>  	struct bpf_prog *old_prog = NULL;
>  	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
>  	struct bpf_cgroup_storage *new_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
> +	struct bpf_cgroup_storage *old_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
>  	struct bpf_prog *new_prog = prog ? : link->link.prog;
>  	enum cgroup_bpf_attach_type atype;
> +	u32 old_flags, old_pl_flags;
>  	struct bpf_prog_list *pl;
>  	struct hlist_head *progs;
>  	int err;
> @@ -865,6 +867,8 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>  
>  	if (pl) {
>  		old_prog = pl->prog;
> +		old_pl_flags = pl->flags;
> +		bpf_cgroup_storages_assign(old_storage, pl->storage);
>  	} else {
>  		pl = kmalloc_obj(*pl);
>  		if (!pl) {
> @@ -884,6 +888,7 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>  	pl->link = link;
>  	pl->flags = flags;
>  	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,15 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>  	if (old_prog) {
>  		pl->prog = old_prog;
>  		pl->link = NULL;
> +		pl->flags = old_pl_flags;
> +		bpf_cgroup_storages_assign(pl->storage, old_storage);
>  	}
>  	bpf_cgroup_storages_free(new_storage);
>  	if (!old_prog) {
>  		hlist_del(&pl->node);
>  		kfree(pl);
>  	}
> +	cgrp->bpf.flags[atype] = old_flags;
>  	return err;
>  }
>