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

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

Pre-existing issues:
- [Critical] When updating a cgroup BPF link via `bpf_link_update`, `__cgroup_bpf_replace` does not allocate new `cgroup_storage` for the new program and lacks map compatibility checks, leading to severe out-of-bounds kernel memory access.
--

--- Patch [1]: [PATCH bpf v3] bpf, cgroup: Fix invalid storage access after __cgroup_bpf_attach failed ---
commit 91e5396c1d1312a251035a5e4902151e56fc5bdf
Author: Pu Lehui <[email protected]>

bpf, cgroup: Fix invalid storage access after __cgroup_bpf_attach failed

This commit fixes a potential invalid storage access issue when replacing a
cgroup BPF program in multi-attach mode fails midway. It saves the old
program's state and storage prior to the update, and properly restores them
in the cleanup path on error.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 4355ccb78a9c6..57fd1d784e440 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c

[ ... ]

While reviewing this file, I noticed a potential issue in __cgroup_bpf_replace():

> static int __cgroup_bpf_replace(struct cgroup *cgrp,
> 				struct bpf_cgroup_link *link,
> 				struct bpf_prog *new_prog)
> {
[ ... ]
> 	cgrp->bpf.revisions[atype] += 1;
> 	old_prog = xchg(&link->link.prog, new_prog);

[Severity: Critical]
This is a pre-existing issue, but does this function properly handle
cgroup_storage when replacing the program?

If a user replaces an existing cgroup BPF program using bpf_link_update with
a new program that uses a BPF_MAP_TYPE_CGROUP_STORAGE map with a larger
value_size, could this result in an out-of-bounds memory access?

It appears we swap the attached BPF program on the link here without
verifying that the new program's cgroup_storage maps have identical
value sizes to the old program, nor do we allocate new appropriately
sized storage buffers.

Since the verifier authorized the memory access for the new program based on
its larger map size, could bpf_get_local_storage return the smaller old
buffer leading to out-of-bounds read and write accesses?

> 	replace_effective_prog(cgrp, atype, pl);
> 	bpf_prog_put(old_prog);
> 	return 0;
> }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1