[PATCH v2] bpf: fix out-of-bounds frame access for async callback stack args

[PATCH v2] bpf: fix out-of-bounds frame access for async callback stack args

[Henry Martin]

check_stack_arg_read() resolves the caller frame with

    caller = vstate->frame[vstate->curframe - 1];

and copies caller->stack_arg_regs[spi] into the current register
state.  It bounds spi against the callee's declared incoming stack
arguments, but nothing validates the caller side.  Two kinds of
callback frames reach this code without a validated caller.

Async callbacks (bpf_timer_set_callback and friends) are queued via
push_async_cb(), which builds a fresh verifier state with frame[0]
as the only frame and curframe left at its kzalloc default 0, so the
code above evaluates frame[-1]: an out-of-bounds read behind the
bpf_verifier_state.frame array whose result is used as a pointer.

Synchronous callbacks (bpf_loop, bpf_for_each_map_elem, ...) run with
curframe >= 1, but push_callback_call() ignores every error from
btf_check_subprog_call() except -EFAULT, so a callback subprogram
whose prototype does not match the helper's callback ABI (e.g. more
than five BTF arguments) is still queued for exploration.  Inside
such a callback an r11-based incoming stack argument load passes the
spi bound check, while the kernel helper never passes stack arguments
at runtime:

  - caller->stack_arg_regs is a heap array sized by the caller's
    out_stack_arg_cnt; if no outgoing stack arguments were prepared,
    the load dereferences NULL or reads past the allocation;
  - if a previous subprogram call left initialized outgoing stack
    arguments behind, the stale register state is copied into the
    callback's registers even though the runtime ABI passes nothing
    there.

Kernel helpers pass at most five callback arguments, all in
registers, so an incoming stack argument load can never be valid in
a callback frame.  Reject such loads when the current frame has no
caller (curframe == 0) or is a callback frame (state->in_callback_fn).

This issue was discovered by Tencent CodeBuddy Security.

Fixes: 0f6bd5e7a804 ("bpf: Support stack arguments for bpf functions")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---

v2: Also reject r11-based incoming stack argument loads in synchronous
    callback frames (state->in_callback_fn), not just the async
    curframe == 0 case: push_callback_call() queues callbacks whose
    prototype failed btf_check_subprog_call() with -EINVAL, leaving
    caller->stack_arg_regs unvalidated or stale (sashiko review).
    The curframe check stays necessary because the async timer and
    workqueue setter set_timer_callback_state() does not set
    in_callback_fn.

---
 kernel/bpf/verifier.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5f874979b8d75..c838ec39590b9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4232,6 +4232,11 @@ static int check_stack_arg_read(struct bpf_verifier_env *env, struct bpf_func_st
 		return -EACCES;
 	}
 
+	if (vstate->curframe == 0 || state->in_callback_fn) {
+		verbose(env, "stack arg read from a callback frame\n");
+		return -EINVAL;
+	}
+
 	caller = vstate->frame[vstate->curframe - 1];
 	arg = &caller->stack_arg_regs[spi];
 	cur = vstate->frame[vstate->curframe];
-- 
2.43.7

Re: [PATCH v2] bpf: fix out-of-bounds frame access for async callback stack args

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: NULL or OOB read of stack_arg_regs in check_stack_arg_read()
--

commit 8bf4e754c411e80ff88d77c6c807380a3c5428eb
Author: Henry Martin <bsdhenrymartin@gmail.com>

bpf: fix out-of-bounds frame access for async callback stack args

This commit rejects r11-based incoming stack argument loads in
check_stack_arg_read() when the current frame has curframe == 0 or
state->in_callback_fn set. This prevents async and synchronous callback
frames from reading unvalidated or out-of-bounds caller frame and stack
argument state.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5601d20562186..c02776c396bc2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4653,6 +4653,11 @@ static int check_stack_arg_read(struct bpf_verifier_env *env, struct bpf_func_st
>  		return -EACCES;
>  	}
>  
> +	if (vstate->curframe == 0 || state->in_callback_fn) {
> +		verbose(env, "stack arg read from a callback frame\n");
> +		return -EINVAL;
> +	}
> +
>  	caller = vstate->frame[vstate->curframe - 1];
>  	arg = &caller->stack_arg_regs[spi];
>  	cur = vstate->frame[vstate->curframe];

[Severity: High]
Can check_stack_arg_read() in kernel/bpf/verifier.c still dereference a NULL
or out-of-bounds caller->stack_arg_regs[spi] (or copy an invalidated NOT_INIT
slot) during a non-callback static subprogram call?

For static subprograms, both check_func_call() and check_func_callx() ignore
all errors from btf_check_subprog_call() except -EFAULT and proceed into
check_static_func_call() with vstate->curframe >= 1 and
state->in_callback_fn == false:

kernel/bpf/verifier.c:check_func_call() {
	...
	caller = state->frame[state->curframe];
	err = btf_check_subprog_call(env, subprog);
	if (err == -EFAULT)
		return err;
	...
}

kernel/bpf/verifier.c:check_func_callx() {
	...
	subprog = reg->subprogno;
	err = btf_check_subprog_call(env, subprog);
	if (err == -EFAULT)
		return err;

	err = record_callx_edge(env, caller->subprogno, subprog);
	if (err)
		return err;

	return check_static_func_call(env, subprog, insn_idx);
}

When a static subprogram has more than five BTF argument slots
(sub->arg_slot_cnt > MAX_BPF_FUNC_REG_ARGS, so bpf_in_stack_arg_cnt(subprog)
is non-zero), btf_check_subprog_call() can return -EINVAL before
check_func_args() -> check_outgoing_stack_args() validates and prepares
caller->stack_arg_regs in two ways:

1. In btf_check_func_arg_match(), after btf_prepare_func_args() succeeds and
populates sub->arg_slot_cnt and sub->stack_arg_cnt, if any register in
caller->regs[BPF_REG_1 .. BPF_REG_5] has type == NOT_INIT, it returns
-EINVAL before calling check_func_args():

kernel/bpf/verifier.c:btf_check_func_arg_match() {
	...
	if (!bpf_subprog_is_global(env, subprog)) {
		for (i = 0; i < min_t(u32, sub->arg_slot_cnt, MAX_BPF_FUNC_REG_ARGS); i++)
			if (caller->regs[BPF_REG_1 + i].type == NOT_INIT)
				return -EINVAL;
	}
	return check_func_args(env, &meta, env->insn_idx);
}

2. In btf_check_subprog_call(), once
prog->aux->func_info_aux[subprog].unreliable is set to true by an earlier
call or callback registration that failed BTF validation, any subsequent call
to that static subprogram returns -EINVAL immediately without calling
btf_check_func_arg_match() or check_outgoing_stack_args():

kernel/bpf/verifier.c:btf_check_subprog_call() {
	...
	if (prog->aux->func_info_aux[subprog].unreliable)
		return -EINVAL;

	err = btf_check_func_arg_match(env, subprog, btf);
	if (err)
		prog->aux->func_info_aux[subprog].unreliable = true;
	return err;
}

When that static subprogram then executes an r11-based incoming stack
argument load, check_stack_arg_read() passes the spi + 1 <=
bpf_in_stack_arg_cnt(subprog) bound check and the new curframe == 0 ||
state->in_callback_fn check, and then reads caller->stack_arg_regs[spi].

If the caller never allocated stack_arg_regs (caller->out_stack_arg_cnt == 0),
this dereferences NULL; if caller->out_stack_arg_cnt <= spi, it reads past
the end of caller->stack_arg_regs; and if a prior call left invalidated slots
behind, it copies a NOT_INIT register state into cur->regs[dst_regno].

Should check_stack_arg_read() also verify that spi < caller->out_stack_arg_cnt
and that caller->stack_arg_regs[spi].type != NOT_INIT (or reject the call
when btf_check_subprog_call() fails for a subprogram with incoming stack
arguments)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027154553.1043567-1-bsdhenrymartin@gmail.com?part=1