Re: [PATCH bpf v3 2/3] bpf, cgroup: Fix storage not restored when update_effective_progs failed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Uninitialized memory is read when saving `pl->storage` into `old_storage` for newly allocated `bpf_prog_list` entries.

Pre-existing issues:
- [High] If the first program attach to a cgroup fails, `cgrp->bpf.flags[atype]` is not rolled back, leading to a complete bypass of ancestor mandatory security policies.
--

--- Patch [2]: [PATCH bpf v3 2/3] bpf, cgroup: Fix storage not restored when update_effective_progs failed ---
commit 30470afb00630f3ed1a914958997d62e12b1f60a
Author: Pu Lehui <pulehui@huawei.com>

bpf, cgroup: Fix storage not restored when update_effective_progs failed

In __cgroup_bpf_attach(), if update_effective_progs() fails, pl->storage
is not restored to old_storage. This patch saves the old_storage before
assigning the new one, and restores it in the error path.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
[ ... ]
> @@ -883,6 +884,7 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>  	pl->prog = prog;
>  	pl->link = link;
>  	pl->flags = flags;
> +	bpf_cgroup_storages_assign(old_storage, pl->storage);

[Severity: Medium]
When __cgroup_bpf_attach() allocates a new bpf_prog_list entry, it does so
without zeroing the memory:

        pl = kmalloc_obj(*pl);

Because pl is not zero-initialized, does calling bpf_cgroup_storages_assign()
here result in reading uninitialized garbage pointers from pl->storage and
copying them into old_storage? This could trigger KMSAN uninit-value warnings
when attaching a new program.

>  	bpf_cgroup_storages_assign(pl->storage, storage);
>  	cgrp->bpf.flags[atype] = saved_flags;
>  
[ ... ]
> @@ -916,6 +918,7 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>  		pl->prog = old_prog;
>  		pl->link = NULL;
>  	}
> +	bpf_cgroup_storages_assign(pl->storage, old_storage);
>  	bpf_cgroup_storages_free(new_storage);
>  	if (!old_prog) {
>  		hlist_del(&pl->node);
>  		kfree(pl);
>  	}
>  	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at the error path, if
the very first program attach to a cgroup fails (for example, if
update_effective_progs() fails), the newly added list entry is deleted and
freed here, making the list empty again.

However, the flags set earlier in the function are not rolled back:

        cgrp->bpf.flags[atype] = saved_flags;

Could an attacker attempt an attach with BPF_F_ALLOW_MULTI, intentionally
force an error (like hitting memory limits in update_effective_progs()), and
leave the compromised cgroup with permanently altered BPF flags? 

If the flags stay stuck, does this allow bypassing mandatory, non-overridable
programs from ancestor cgroups during compute_effective_progs()?

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