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

[bot+bpf-ci]

> 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