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

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

[Pu Lehui]

From: Pu Lehui <pulehui@huawei.com>

Sashiko reported a potential invalid storage access issue after
replacing a cgroup bpf prog.

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@kernel.org>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
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@huaweicloud.com

 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;
 }
 
-- 
2.34.1

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@huawei.com>
>
> 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@kernel.org>
> Signed-off-by: Pu Lehui <pulehui@huawei.com>

Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>

> ---
> 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@huaweicloud.com
>
>  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;
>  }
>  

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

[Pu Lehui]

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 <pulehui@huawei.com>
>>
>> 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 <sashiko-bot@kernel.org>
>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> 
> Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>
> 
>> ---
>> 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@huaweicloud.com
>>
>>   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;
>>   }
>>