[PATCH bpf] bpf: Enforce cgroup storage map consistency for freplace attach

[PATCH bpf] bpf: Enforce cgroup storage map consistency for freplace attach

[Aohan Mei]

From: Aohan Mei <henrymei@tencent.com>

When a BPF_PROG_TYPE_EXT program replaces a cgroup program, it
executes with the target's runtime context, including the per-program
cgroup storage descriptor attached to the cgroup prog item. The
verifier, however, bounds the extension's bpf_get_local_storage()
accesses by the extension's own storage map.

The prog-array path already enforces that programs sharing a
runtime storage context reference identical storage maps (via the
owner cookie matching added in commit abad3d0bad72 ("bpf: Fix oob
access in cgroup local storage")), but the freplace path performs
no such consistency check in bpf_freplace_check_tgt_prog(). An
extension whose storage map differs from the target's therefore
operates on a buffer whose layout does not match its verified
bounds.

Reject the freplace attach with -EINVAL when the extension and the
target program reference mismatched cgroup storage maps.

Fixes: 7d9c3427894f ("bpf: Make cgroup storages shared between programs on the same cgroup")
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Cc: stable@vger.kernel.org
Assisted-by: CodeBuddy:Kimi-K3
Signed-off-by: Aohan Mei <henrymei@tencent.com>
---
 kernel/bpf/trampoline.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 1a721fc4bef5..3743f6c25e2c 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -806,9 +806,11 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
 	}
 }
 
-static int bpf_freplace_check_tgt_prog(struct bpf_prog *tgt_prog)
+static int bpf_freplace_check_tgt_prog(struct bpf_prog *tgt_prog,
+				       struct bpf_prog *prog)
 {
 	struct bpf_prog_aux *aux = tgt_prog->aux;
+	enum bpf_cgroup_storage_type i;
 
 	guard(mutex)(&aux->ext_mutex);
 	if (aux->prog_array_member_cnt)
@@ -821,6 +823,20 @@ static int bpf_freplace_check_tgt_prog(struct bpf_prog *tgt_prog)
 		return -EBUSY;
 
 	aux->is_extended = true;
+
+	/* At runtime the extension program inherits the target program's
+	 * cgroup storage context (via prog_item->cgroup_storage), while the
+	 * verifier bounds its accesses by its own map's value_size. A
+	 * value_size mismatch leads to slab out-of-bounds access.
+	 */
+	for_each_cgroup_storage_type(i) {
+		struct bpf_map *tgt_map = tgt_prog->aux->cgroup_storage[i];
+		struct bpf_map *prog_map = prog->aux->cgroup_storage[i];
+
+		if (prog_map && (!tgt_map || prog_map->value_size != tgt_map->value_size))
+			return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -926,7 +942,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_node *node,
 		/* Cannot attach extension if fentry/fexit are in use. */
 		if (cnt)
 			return -EBUSY;
-		err = bpf_freplace_check_tgt_prog(tgt_prog);
+		err = bpf_freplace_check_tgt_prog(tgt_prog, node->link->prog);
 		if (err)
 			return err;
 		tr->extension_prog = node->link->prog;
-- 
2.43.7

Re: [PATCH bpf] bpf: Enforce cgroup storage map consistency for freplace attach

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] State corruption in bpf_freplace_check_tgt_prog() leaking aux->is_extended = true on validation failure.
--

--- Patch [1]: [PATCH bpf] bpf: Enforce cgroup storage map consistency for freplace attach ---
commit 25f8d74e87a11313024cd3ea152a45bd57843da3
Author: Aohan Mei <henrymei@tencent.com>
Subject: bpf: Enforce cgroup storage map consistency for freplace attach

This commit adds a consistency check for cgroup storage maps when attaching
an extension program. It rejects the attach with -EINVAL if the extension
and target programs reference mismatched cgroup storage maps, preventing
potential out-of-bounds access.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
[ ... ]
> @@ -821,6 +823,20 @@ static int bpf_freplace_check_tgt_prog(struct bpf_prog *tgt_prog)
>  		return -EBUSY;
>  
>  	aux->is_extended = true;
> +
> +	/* At runtime the extension program inherits the target program's
> +	 * cgroup storage context (via prog_item->cgroup_storage), while the
> +	 * verifier bounds its accesses by its own map's value_size. A
> +	 * value_size mismatch leads to slab out-of-bounds access.
> +	 */
> +	for_each_cgroup_storage_type(i) {
> +		struct bpf_map *tgt_map = tgt_prog->aux->cgroup_storage[i];
> +		struct bpf_map *prog_map = prog->aux->cgroup_storage[i];
> +
> +		if (prog_map && (!tgt_map || prog_map->value_size != tgt_map->value_size))
> +			return -EINVAL;

[Severity: High]
If this check fails and returns -EINVAL, does it leave aux->is_extended
permanently set to true?

Because aux->is_extended is set to true just before this loop, returning
an error here without reverting it might permanently prevent the target program
from being used as a tail callee, since prog_fd_array_get_ptr() strictly
rejects programs with is_extended == true.

Should this error path revert aux->is_extended to false?

> +	}
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_2EB11B9074207E825BE3ACC0AF1F09DE0108@qq.com?part=1

Re: [PATCH bpf] bpf: Enforce cgroup storage map consistency for freplace attach

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3502 bytes --]

> The prog-array path already enforces that programs sharing a
> runtime storage context reference identical storage maps (via the
> owner cookie matching added in commit abad3d0bad72 ("bpf: Fix oob
> access in cgroup local storage")), but the freplace path performs
> no such consistency check in bpf_freplace_check_tgt_prog(). An
> extension whose storage map differs from the target's therefore
> operates on a buffer whose layout does not match its verified
> bounds.
>
> Reject the freplace attach with -EINVAL when the extension and the
> target program reference mismatched cgroup storage maps.

The changelog describes the check as enforcing "identical storage maps"
(matching the prog-array path's owner cookie comparison), but the code
compares only value_size:

    if (prog_map && (!tgt_map || prog_map->value_size != tgt_map->value_size))
            return -EINVAL;

Two different cgroup storage maps with equal value_size are accepted.
The precedent cited, __bpf_prog_map_compatible() in kernel/bpf/core.c,
matches map identity via map->owner->storage_cookie[i], not size.

The value_size comparison is correct and sufficient for preventing the
out-of-bounds access (bpf_get_local_storage() returns storage sized from
the owning map's value_size, while the verifier bounds the extension by
its own map's value_size), so this isn't a memory-safety gap.

With equal value_size the extension still operates on the target's
storage, e.g. an extension whose map lacks BPF_F_RDONLY_PROG can write
into a target storage map created with BPF_F_RDONLY_PROG
(LOCAL_STORAGE_CREATE_FLAG_MASK includes BPF_F_ACCESS_MASK in
kernel/bpf/local_storage.c), and value layouts may differ (BPF_SPIN_LOCK
is permitted for BPF_MAP_TYPE_CGROUP_STORAGE per kernel/bpf/syscall.c).

Should the check be tightened to map identity (matching the cited
commit), or should the changelog be reworded to say the check only
rejects value_size mismatches?

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 1a721fc4bef5..3743f6c25e2c 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c

[ ... ]

> @@ -821,6 +823,20 @@ static int bpf_freplace_check_tgt_prog(struct bpf_prog *tgt_prog)
>  		return -EBUSY;
>
>  	aux->is_extended = true;
> +
> +	/* At runtime the extension program inherits the target program's
> +	 * cgroup storage context (via prog_item->cgroup_storage), while the
> +	 * verifier bounds its accesses by its own map's value_size. A
> +	 * value_size mismatch leads to slab out-of-bounds access.
> +	 */

This isn't a bug, but the comment formatting differs from the BPF
subsystem style. Under kernel/bpf/, multi-line comments place the
opening /* on its own line:

    /*
     * At runtime the extension program inherits the target program's
     * ...
     */

The pre-existing comment just above (around the function start) uses the
same style as the new comment, so this may be intentional consistency
with the file.

> +	for_each_cgroup_storage_type(i) {
> +		struct bpf_map *tgt_map = tgt_prog->aux->cgroup_storage[i];
> +		struct bpf_map *prog_map = prog->aux->cgroup_storage[i];
> +
> +		if (prog_map && (!tgt_map || prog_map->value_size != tgt_map->value_size))
> +			return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>

---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31804018762