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

On 2026/7/30 14:33, Emil Tsalapatis wrote:
> On Wed Jul 29, 2026 at 6:02 AM EDT, Pu Lehui wrote:
>> From: Pu Lehui <[email protected]>
>>
>> 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.

alright, will respin soon.

> 
>> 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 <[email protected]>
>> Signed-off-by: Pu Lehui <[email protected]>
> 
> Reviewed-by: Emil Tsalapatis <[email protected]>
> 
>> ---
>> 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/[email protected]
>>
>>   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;
>>   }
>>