[PATCH bpf] bpf: guard classic uprobes against private-stack corruption

[PATCH bpf] bpf: guard classic uprobes against private-stack corruption

[Christian Simon]

Eligible BPF programs use one private stack per program and CPU.
bpf_prog_run_array_uprobe() uses migrate_disable() to keep an
invocation on one CPU, but another task can still preempt it and run
the same program on that CPU. The second invocation then reuses and
can overwrite the first invocation's private stack.

Protect each real program invocation with the existing per-program
recursion context. When the program is already active on this CPU,
account for the missed invocation and skip it. This matches the
private-stack protection in bpf_prog_run_array_sleepable().

Skip dummy_bpf_prog before acquiring the recursion context because its
active pointer is NULL.

Fixes: 7d1cd70d4b16 ("bpf, x86: Support private stack in jit")
Closes: https://github.com/open-telemetry/opentelemetry-ebpf-instrumentation/issues/3056
Cc: stable@vger.kernel.org
Signed-off-by: Christian Simon <simon@swine.de>
---

This is my first contribution to the BPF subsystem. I reproduced the
failure described in the linked report and would particularly appreciate
review of the recursion-context handling.

I have a reproducer, that this patch fixes, I am also unsure if this is
something that should be part of the selftests.

Tests: https://github.com/simonswine/beyla/commit/20c9912a02c5d5924c992faf7ee353c7d71c9fa0 


The analogous uprobe-multi path appears affected but is not addressed by
this patch.


 include/linux/bpf.h | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7719f6528445..11e3d7f00543 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2572,12 +2572,20 @@ static inline void bpf_reset_run_ctx(struct bpf_run_ctx *old_ctx)
 
 typedef u32 (*bpf_prog_run_fn)(const struct bpf_prog *prog, const void *ctx);
 
+#ifdef CONFIG_BPF_SYSCALL
+void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
+#else
+static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
+{
+}
+#endif
+
 static __always_inline u32
 bpf_prog_run_array(const struct bpf_prog_array *array,
 		   const void *ctx, bpf_prog_run_fn run_prog)
 {
 	const struct bpf_prog_array_item *item;
-	const struct bpf_prog *prog;
+	struct bpf_prog *prog;
 	struct bpf_run_ctx *old_run_ctx;
 	struct bpf_trace_run_ctx run_ctx;
 	u32 ret = 1;
@@ -2635,15 +2643,30 @@ bpf_prog_run_array_uprobe(const struct bpf_prog_array *array,
 	old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
 	item = &array->items[0];
 	while ((prog = READ_ONCE(item->prog))) {
+		/* dummy_bpf_prog has no recursion state. */
+		if (unlikely(!prog->len)) {
+			item++;
+			continue;
+		}
+
+		if (unlikely(!bpf_prog_get_recursion_context(prog))) {
+			bpf_prog_inc_misses_counter(prog);
+			bpf_prog_put_recursion_context(prog);
+			item++;
+			continue;
+		}
+
 		if (!prog->sleepable)
 			rcu_read_lock();
 
 		run_ctx.bpf_cookie = item->bpf_cookie;
 		ret &= run_prog(prog, ctx);
-		item++;
 
 		if (!prog->sleepable)
 			rcu_read_unlock();
+
+		bpf_prog_put_recursion_context(prog);
+		item++;
 	}
 	bpf_reset_run_ctx(old_run_ctx);
 	migrate_enable();
@@ -3208,8 +3231,6 @@ static inline bool has_current_bpf_ctx(void)
 	return !!current->bpf_ctx;
 }
 
-void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
-
 void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
 		     enum bpf_dynptr_type type, u32 offset, u32 size);
 void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr);
@@ -3538,10 +3559,6 @@ static inline bool has_current_bpf_ctx(void)
 	return false;
 }
 
-static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
-{
-}
-
 static inline void bpf_cgrp_storage_free(struct cgroup *cgroup)
 {
 }
-- 
2.54.0

Re: [PATCH bpf] bpf: guard classic uprobes against private-stack corruption

[sashiko-bot]

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

New issues:
- [Low] The patch causes a `-Wdiscarded-qualifiers` compiler warning in `bpf_prog_run_array_uprobe` and mistakenly removes `const` from `bpf_prog_run_array`.

Pre-existing issues:
- [High] Missing recursion context protection in the uprobe-multi path leads to per-CPU private stack corruption.
--

--- Patch [1]: [PATCH bpf] bpf: guard classic uprobes against private-stack corruption ---
commit d583ad326b3c9bb2b3cfd745309987d581a65249
Author: Christian Simon <simon@swine.de>

bpf: guard classic uprobes against private-stack corruption

Eligible BPF programs use one private stack per program and CPU.
bpf_prog_run_array_uprobe() uses migrate_disable() to keep an
invocation on one CPU, but another task can still preempt it and run
the same program on that CPU. The second invocation then reuses and
can overwrite the first invocation's private stack.

> The analogous uprobe-multi path appears affected but is not addressed by
> this patch.

[Severity: High]
This is a pre-existing issue, but since the uprobe-multi path shares this
vulnerability, shouldn't it be addressed as well to prevent private-stack
corruption?

In kernel/trace/bpf_trace.c:uprobe_prog_run(), the BPF program is executed
without recursion context protection:

        err = bpf_prog_run(link->link.prog, regs);

If a task executing uprobe_prog_run() is preempted (or yields) and the
preempting task triggers the same program on the same CPU, the private stack
can be overwritten, breaking verifier safety guarantees. Could we add
bpf_prog_get_recursion_context() protection to uprobe_prog_run() too?

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index ffa5626411ac2..1d263992213e1 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
[ ... ]
>  static __always_inline u32
>  bpf_prog_run_array(const struct bpf_prog_array *array,
>  		   const void *ctx, bpf_prog_run_fn run_prog)
>  {
>  	const struct bpf_prog_array_item *item;
> -	const struct bpf_prog *prog;
> +	struct bpf_prog *prog;

[Severity: Low]
Was this const removal intended for bpf_prog_run_array_uprobe() instead?

Removing the const qualifier here doesn't seem necessary for
bpf_prog_run_array().

[ ... ]
> @@ -2640,15 +2648,30 @@ bpf_prog_run_array_uprobe(const struct bpf_prog_array *array,
>  	old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
>  	item = &array->items[0];
>  	while ((prog = READ_ONCE(item->prog))) {
> +		/* dummy_bpf_prog has no recursion state. */
> +		if (unlikely(!prog->len)) {
> +			item++;
> +			continue;
> +		}
> +
> +		if (unlikely(!bpf_prog_get_recursion_context(prog))) {

[Severity: Low]
Does passing prog to bpf_prog_get_recursion_context() here cause a
build regression with a -Wdiscarded-qualifiers compiler warning?

Since prog is declared as const struct bpf_prog *prog in
bpf_prog_run_array_uprobe(), and bpf_prog_get_recursion_context() expects a
non-const pointer, this discards the const qualifier.

Should the const removal above have been applied here in
bpf_prog_run_array_uprobe() instead?

> +			bpf_prog_inc_misses_counter(prog);
> +			bpf_prog_put_recursion_context(prog);
> +			item++;
> +			continue;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818095545.314358-1-simon@swine.de?part=1