Re: [PATCH bpf v6 4/4] bpf, cgroup: Fix storage null-ptr-deref after replacing prog

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing rollback of `pl->flags` on the error path in `__cgroup_bpf_attach` permanently corrupts the execution order of the replaced program.
--

--- Patch [4]: [PATCH bpf v6 4/4] bpf, cgroup: Fix storage null-ptr-deref after replacing prog ---
commit f8726e124d0a4e72b87976830f0741e286d474df
Author: Pu Lehui <pulehui@huawei.com>

bpf, cgroup: Fix storage null-ptr-deref after replacing prog

Syzkaller reported a storage null-ptr-deref issue after replacing a prog.
The issue is fixed by properly allocating the storage and comparing the old
and new storage pointers, falling back to an RCU-safe update if they change.
Additionally, proper rollbacks for storage and flags are added to the error
path in __cgroup_bpf_attach for code rigor.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -883,7 +885,10 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>  	pl->prog = prog;
>  	pl->link = link;
>  	pl->flags = flags;
> +	if (old_prog)
> +		bpf_cgroup_storages_assign(old_storage, pl->storage);
>  	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,14 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
>  	if (old_prog) {
>  		pl->prog = old_prog;
>  		pl->link = NULL;
> +		bpf_cgroup_storages_assign(pl->storage, old_storage);
>  	}

[Severity: High]
This is a pre-existing issue, but since this patch tightens the error handling
and rollbacks in __cgroup_bpf_attach(), should we also restore the original
pl->flags here?

Earlier in the function, pl->flags is overwritten with the incoming flags:

	pl->prog = prog;
	pl->link = link;
	pl->flags = flags;

If update_effective_progs() later fails (e.g., due to memory allocation
failure), the error path restores pl->prog, pl->link, and pl->storage, but
misses pl->flags. If a user attempts to replace a program using BPF_F_REPLACE
with different BPF_F_PREORDER settings and the replacement fails, could this
leave the original program active but with its execution order silently
corrupted?

>  	bpf_cgroup_storages_free(new_storage);
>  	if (!old_prog) {
>  		hlist_del(&pl->node);
>  		kfree(pl);
>  	}
> +	cgrp->bpf.flags[atype] = old_flags;
>  	return err;
>  }

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