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

[PATCH bpf v2] 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:
bpf_get_local_storage() resolves the buffer via
prog_item->cgroup_storage, so the extension's own storage map never
provides storage at runtime. 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 size, flags and layout do not match its
verified assumptions: a smaller target buffer leads to slab
out-of-bounds access, and even equal-sized maps can bypass
BPF_F_RDONLY_PROG or mismatch BPF_SPIN_LOCK fields.

Reject the freplace attach with -EINVAL unless the extension
references the exact same cgroup storage map as the target program,
matching the cookie semantics of __bpf_prog_map_compatible().

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 | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Changes in v2:
- Tighten the check from a value_size comparison to map identity:
  the extension's own map never provides storage at runtime, so a
  different map can only create verifier/runtime inconsistencies
  (BPF_F_RDONLY_PROG bypass, BPF_SPIN_LOCK layout mismatch), pointed
  out by the bpf CI review.
- Reformat the added comment to the kernel multi-line style.

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 1a721fc4bef5..043bee6aaab2 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,23 @@ 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), so its
+	 * own map never provides storage; the verifier, however, bounds
+	 * its accesses (size, flags, layout) by that map. Require both
+	 * programs to reference the same map, matching the cookie
+	 * semantics of the prog-array path.
+	 */
+	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 && prog_map != tgt_map)
+			return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -926,7 +945,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 v2] bpf: Enforce cgroup storage map consistency for freplace attach

[sashiko-bot]

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

New issues:
- [High] The function `bpf_freplace_check_tgt_prog` leaks `tgt_prog->aux->is_extended = true` if the cgroup storage map consistency check fails.

Pre-existing issues:
- [High] Failure in `bpf_arch_text_poke` during freplace attachment corrupts trampoline state and leaves a dangling pointer, permanently blocking future attachments.
--

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

bpf: Enforce cgroup storage map consistency for freplace attach

This commit adds a check in bpf_freplace_check_tgt_prog() to ensure that an
extension program references the exact same cgroup storage map as the target
program. If the maps differ, it returns -EINVAL to prevent verifier and
runtime inconsistencies.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 1a721fc4bef56..043bee6aaab2a 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c

[ ... ]

> @@ -821,6 +823,23 @@ 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), so its
> +	 * own map never provides storage; the verifier, however, bounds
> +	 * its accesses (size, flags, layout) by that map. Require both
> +	 * programs to reference the same map, matching the cookie
> +	 * semantics of the prog-array path.
> +	 */
> +	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 && prog_map != tgt_map)
> +			return -EINVAL;

[Severity: High]
Does this code leak the aux->is_extended state?

If the cgroup storage map consistency check fails here and returns -EINVAL,
aux->is_extended remains permanently set to true. Will this cause future
attempts to add the target program to a prog_array map to fail with -EBUSY?

> +	}
> +
>  	return 0;
>  }

[ ... ]

> @@ -926,7 +945,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;
> 		return bpf_arch_text_poke(tr->func.addr, BPF_MOD_NOP,
> 					  BPF_MOD_JUMP, NULL,
> 					  node->link->prog->bpf_func);

[Severity: High]
This is a pre-existing issue, but if bpf_arch_text_poke() fails here, does
it leave tr->extension_prog pointing to a failed program?

If the text modification fails and returns an error, the failed link will
subsequently be cleaned up and freed, which might leave tr->extension_prog
as a dangling pointer. Furthermore, tgt_prog->aux->is_extended is left as
true. Would future attachments to this trampoline see the non-NULL dangling
pointer and fail with -EBUSY?

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

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

[bot+bpf-ci]

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

> bpf: Enforce cgroup storage map consistency for freplace attach
>
> 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:
> bpf_get_local_storage() resolves the buffer via
> prog_item->cgroup_storage, so the extension's own storage map never
> provides storage at runtime. 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 size, flags and layout do not match its
> verified assumptions: a smaller target buffer leads to slab
> out-of-bounds access, and even equal-sized maps can bypass
> BPF_F_RDONLY_PROG or mismatch BPF_SPIN_LOCK fields.
>
> Reject the freplace attach with -EINVAL unless the extension
> references the exact same cgroup storage map as the target program,
> matching the cookie semantics of __bpf_prog_map_compatible().
>
> Fixes: 7d9c3427894f ("bpf: Make cgroup storages shared between programs on the same cgroup")

Does this Fixes tag point to the correct commit? Looking at the commit
history, 7d9c3427894f introduced storage map sharing between programs on
the same cgroup.

However, before that commit, bpf_cgroup_storage_assign() bound each
cgroup storage map to exactly one program. This meant a target program and
an extension that both used cgroup storage were guaranteed to reference
different maps - the mismatch this patch rejects was unavoidable.

The extension's bpf_get_local_storage() already resolved the buffer from
the runtime prog-array item rather than from its own map even before
7d9c3427894f, so the underlying behavior existed earlier.

The condition became reachable when dynamic program extensions were
introduced in be8704ff07d2 ("bpf: Introduce dynamic program extensions").
Since this is Cc'd to stable, should the Fixes tag reference be8704ff07d2
instead to direct the backport to the correct range?


---
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/32002984717