Re: [PATCH bpf] bpf, cgroup: Fix potential UAF in __cgroup_bpf_attach

[Pu Lehui <pulehui@huawei.com>]

On 2026/7/28 21:23, Pu Lehui wrote:
> From: Pu Lehui <pulehui@huawei.com>
> 
> When replacing an existing bpf_link in single-attach case in
> __cgroup_bpf_attach, old_prog is NULL while old_link holds the active
> link. If attachment fails midway (bpf_trampoline_link_cgroup_shim or
> update_effective_progs), the cleanup check `if (!old_prog)` wrongly
> treats the existing pl entry as newly allocated, calling kfree() and
> causing a UAF.
> 
> Additionally, original flags and storage state are not restored on
> failure.
> 
> Fix this by checking `!old_prog && !old_link` before freeing pl, and
> properly restoring all previous states on error.

Nack, sorry for the noise.

link attach only support BPF_F_ALLOW_MULTI mode, so it will not occur 
UAF pl.

But there still remain an invalid storage access issue. I have been 
mentioned and fix it in v2. Hope that will fix.

https://lore.kernel.org/bpf/20260729100208.3076769-1-pulehui@huaweicloud.com/T/#u

> 
> Fixes: af6eea57437a ("bpf: Implement bpf_link-based cgroup BPF program attachment")
> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> ---
>   kernel/bpf/cgroup.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index e2fa0ebeed83..e22e7dfe0022 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -811,10 +811,13 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>   {
>   	u32 saved_flags = (flags & (BPF_F_ALLOW_OVERRIDE | BPF_F_ALLOW_MULTI));
>   	struct bpf_prog *old_prog = NULL;
> +	struct bpf_cgroup_link *old_link = 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 +868,9 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>   
>   	if (pl) {
>   		old_prog = pl->prog;
> +		old_link = pl->link;
> +		old_pl_flags = pl->flags;
> +		bpf_cgroup_storages_assign(old_storage, pl->storage);
>   	} else {
>   		pl = kmalloc_obj(*pl);
>   		if (!pl) {
> @@ -884,6 +890,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) {
> @@ -912,15 +919,18 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>   		bpf_trampoline_unlink_cgroup_shim(new_prog);
>   
>   cleanup:
> -	if (old_prog) {
> +	if (old_prog || old_link) {
>   		pl->prog = old_prog;
> -		pl->link = NULL;
> +		pl->link = old_link;
> +		pl->flags = old_pl_flags;
> +		bpf_cgroup_storages_assign(pl->storage, old_storage);
>   	}
>   	bpf_cgroup_storages_free(new_storage);
> -	if (!old_prog) {
> +	if (!old_prog && !old_link) {
>   		hlist_del(&pl->node);
>   		kfree(pl);
>   	}
> +	cgrp->bpf.flags[atype] = old_flags;
>   	return err;
>   }
>