[PATCH bpf 1/2] bpf: Validate stack arg reads against caller state

[PATCH bpf 1/2] bpf: Validate stack arg reads against caller state

[Weiming Shi]

check_stack_arg_read() bounds a stack argument index with the callee's
BTF-derived argument count, then uses the index in the caller's
stack_arg_regs[] array. The array's actual logical length is recorded in
caller->out_stack_arg_cnt, so the bound and indexed object can diverge. A
crafted BPF_PROG_LOAD can use an argument mismatch to mark a static
subprogram's BTF information unreliable. Later calls return from
btf_check_subprog_call() before check_outgoing_stack_args(), while
check_func_call() continues the static call on -EINVAL. The callee can
then read a slot that the caller never allocated or initialized. Async
callback verification also starts at curframe zero, so subtracting one
to find a caller can underflow the frame array.

On x86-64, KASAN reports the resulting out-of-bounds copy as:

  BUG: KASAN: slab-out-of-bounds in check_load_mem+0xb51/0xc60
  Read of size 80 at addr ffff8881009af550 by task poc/131
  Call Trace:
   check_stack_arg_read at kernel/bpf/verifier.c:4234
   (inlined by) check_load_mem at kernel/bpf/verifier.c:6645
   do_check_common at kernel/bpf/verifier.c:19568
   bpf_check at kernel/bpf/verifier.c:21318
   bpf_prog_load at kernel/bpf/syscall.c:3134

The copy replaces the destination with a complete struct bpf_reg_state.
A deterministic reproducer retained a PTR_TO_MAP_VALUE state in
allocation slack, imported it into R0 through this read, and made the
verifier accept a following load through R0.

Require a caller frame before indexing it and retrieve the slot with
bpf_get_spilled_stack_arg(). The helper validates the index against
caller->out_stack_arg_cnt and rejects uninitialized slots before the
register state is copied.

Fixes: 0f6bd5e7a804 ("bpf: Support stack arguments for bpf functions")
Reported-by: <co+eada64ce90ecebce@bugs.sh>
Assisted-by: LLM
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 kernel/bpf/verifier.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5f874979b..a67195aa7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4232,8 +4232,18 @@ static int check_stack_arg_read(struct bpf_verifier_env *env, struct bpf_func_st
 		return -EACCES;
 	}
 
+	if (!vstate->curframe) {
+		verbose(env, "invalid read from stack arg off %d without caller\n", off);
+		return -EACCES;
+	}
+
 	caller = vstate->frame[vstate->curframe - 1];
-	arg = &caller->stack_arg_regs[spi];
+	arg = bpf_get_spilled_stack_arg(spi, caller);
+	if (!arg) {
+		verbose(env, "invalid read from uninitialized stack arg off %d\n", off);
+		return -EACCES;
+	}
+
 	cur = vstate->frame[vstate->curframe];
 	bpf_diag_mod_begin(env, &cur->regs[dst_regno], arg, BPF_DIAG_MOD_WRITE);
 	cur->regs[dst_regno] = *arg;
-- 
2.55.0

[PATCH bpf 2/2] selftests/bpf: Test missing caller stack argument slots

[Weiming Shi]

Exercise the path where a static seven-argument subprogram has
unreliable BTF and is called through a bridge with only one initialized
outgoing stack argument. The callee expects two stack arguments, but the
unreliable-BTF path skips check_outgoing_stack_args().

Without the fix, the second r11 load copies an 80-byte bpf_reg_state from
stack_arg_regs[1], past the caller's one-element array. Expect the
verifier to reject that load at offset 16.

The fix also rejects the existing uninitialized-stack-argument test
before the later unreadable-register check. Update that test's expected
diagnostic accordingly.

Assisted-by: LLM
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 .../bpf/progs/verifier_stack_arg_order.c      | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_arg_order.c b/tools/testing/selftests/bpf/progs/verifier_stack_arg_order.c
index c9fe4857d..23c7515ea 100644
--- a/tools/testing/selftests/bpf/progs/verifier_stack_arg_order.c
+++ b/tools/testing/selftests/bpf/progs/verifier_stack_arg_order.c
@@ -129,6 +129,16 @@ static void subprog_bad_ptr_7args(long *a, int b, int c, int d, int e, int f, in
 	);
 }
 
+__noinline __used __naked
+static void subprog_call_bad_ptr_7args(void)
+{
+	asm volatile ("*(u64 *)(r11 - 8) = 0;"
+		"call subprog_bad_ptr_7args;"
+		"exit;"
+		::: __clobber_all
+	);
+}
+
 SEC("tc")
 __description("stack_arg: read without caller write")
 __failure
@@ -151,7 +161,7 @@ __naked void stack_arg_read_without_write_1(void)
 SEC("tc")
 __description("stack_arg: read with not-initialized caller write")
 __failure
-__msg("R0 !read_ok")
+__msg("invalid read from uninitialized stack arg off 8")
 __btf_func_path("btf__verifier_stack_arg_order.bpf.o")
 __naked void stack_arg_read_without_write_2(void)
 {
@@ -170,6 +180,27 @@ __naked void stack_arg_read_without_write_2(void)
 	);
 }
 
+SEC("tc")
+__description("stack_arg: unreliable callee with too few stack args")
+__failure
+__msg("invalid read from uninitialized stack arg off 16")
+__btf_func_path("btf__verifier_stack_arg_order.bpf.o")
+__naked void stack_arg_read_with_short_bridge_write(void)
+{
+	asm volatile ("r1 = 0;"
+		"r2 = 0;"
+		"r3 = 0;"
+		"r4 = 0;"
+		"r5 = 0;"
+		"*(u64 *)(r11 - 8) = 0;"
+		"*(u64 *)(r11 - 16) = 0;"
+		"call subprog_bad_ptr_7args;"
+		"call subprog_call_bad_ptr_7args;"
+		"exit;"
+		::: __clobber_all
+	);
+}
+
 #else
 
 SEC("socket")
-- 
2.55.0

Re: [PATCH bpf 1/2] bpf: Validate stack arg reads against caller state

[bot+bpf-ci]

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

> bpf: Validate stack arg reads against caller state

TL;DR: Pre-existing, not from this patch: a synchronous callback subprog (e.g.
bpf_loop) with more than 5 args may still read the helper caller's stack arg
slots, so the verifier could accept unsafe reads. Should callbacks be
rejected?

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5f874979b8d7..a67195aa7ce7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4232,8 +4232,18 @@ static int check_stack_arg_read(struct bpf_verifier_env *env, struct bpf_func_st
>  		return -EACCES;
>  	}
>  
> +	if (!vstate->curframe) {
> +		verbose(env, "invalid read from stack arg off %d without caller\n", off);
> +		return -EACCES;
> +	}
> +
>  	caller = vstate->frame[vstate->curframe - 1];
> -	arg = &caller->stack_arg_regs[spi];
> +	arg = bpf_get_spilled_stack_arg(spi, caller);
> +	if (!arg) {
> +		verbose(env, "invalid read from uninitialized stack arg off %d\n", off);
> +		return -EACCES;
> +	}

This isn't introduced by this patch, but the same class of problem looks
like it is still reachable through synchronous callbacks.  Can a callback
subprog still read a stack arg slot that the helper's caller wrote?

Synchronous callbacks (bpf_loop, bpf_for_each_map_elem, user_ringbuf_drain,
callback kfuncs) are verified through push_callback_call() ->
setup_func_entry().  There, frame[curframe - 1] is the frame that called the
helper, and its stack_arg_regs[] still holds whatever it wrote before the
helper call.  check_helper_call() only runs invalidate_outgoing_stack_args()
after push_callback_call() has already copied the state with push_stack().

At runtime the callback is not called by that frame.  bpf_loop() in
kernel/bpf/bpf_iter.c does:

    callback((u64)i, (u64)(long)callback_ctx, 0, 0, 0)

with no stack arguments.  The x86-64 JIT reads incoming arg6 from native r9
and arg7 onwards from [rbp+16...], so the callback would read a caller-saved
register and the helper's own stack frame.

A possible sequence:

  - A static callback whose BTF has more than 5 args gets sub->arg_cnt > 5
    from btf_prepare_func_args().  Nothing restricts callback arg counts.

  - The program writes r11-8/r11-16 (for example a PTR_TO_MAP_VALUE) and
    calls bpf_loop().  The first btf_check_subprog_call() passes
    check_outgoing_stack_args().

  - The register type mismatch on R2 (PTR_TO_FUNC) only marks the callback
    unreliable and returns -EINVAL, which push_callback_call() ignores.

  - In the callback, this passes every check added here:

        r0 = *(u64 *)(r11 + 16)

    spi + 1 <= bpf_in_stack_arg_cnt(), curframe == 1, and
    bpf_get_spilled_stack_arg() finds an initialized slot.

R0 then gets the caller's PTR_TO_MAP_VALUE state while the runtime value
comes from the helper's stack.  bpf_fixup_call_args() is satisfied as long
as the main program also makes one ordinary call to a 7-arg static subprog.

Should check_stack_arg_read() (or push_callback_call()) reject in-stack args
when the current frame is a callback (cur->in_callback_fn), or should
callback subprogs with more than MAX_BPF_FUNC_REG_ARGS args be rejected
outright?

The later commit in the series, b6256658f774, only adds selftests, so it
doesn't appear to address this.


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