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

[PATCH] 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];

without checking whether the current frame is the outermost one
(curframe == 0). Regular programs always run with curframe > 0 inside
check_stack_arg_read(), but async callbacks do not: push_async_cb()
builds a fresh verifier state with frame[0] as the only frame and
curframe left at its kzalloc default 0.

A static subprogram invoked as an async callback (e.g. via
bpf_timer_set_callback) may declare up to MAX_BPF_FUNC_ARGS (12)
arguments, and arguments beyond the fifth are passed on the stack. When
such a callback executes an r11-based incoming stack argument load
(is_stack_arg_ldx: src_reg == BPF_REG_PARAMS, off > 0, 8-aligned), the
spi bound check against bpf_in_stack_arg_cnt() passes while curframe is
0, and the code above evaluates frame[-1]: an out-of-bounds read behind
the bpf_verifier_state.frame array. frame[] is embedded in struct
bpf_verifier_state, so for async callbacks frame[-1] reads the last 8
bytes of the object preceding the stack element in the verifier state
allocator, and the resulting garbage pointer is immediately used as

    arg = &caller->stack_arg_regs[spi];

whose contents are copied into the verifier register state. The main
program path is not affected: do_check_common() forces arg_cnt == 1 for
subprog 0, so bpf_in_stack_arg_cnt() is 0 there and the bound check
rejects the load.

Reject r11-based incoming stack argument loads when the current frame
has no caller (curframe == 0): async callback prototypes cannot declare
stack arguments in the first place (e.g. the timer callback prototype
has only 3 arguments), so such a load can never be valid.

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>
---
 kernel/bpf/verifier.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5f874979b8d75..1fb3e89ac59b5 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) {
+		verbose(env, "stack arg read with no caller 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] 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:
- [Critical] bpf: incomplete caller stack_arg_regs validation in check_stack_arg_read()
--

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

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

This commit adds a check in check_stack_arg_read() to reject r11-based
incoming stack argument loads when vstate->curframe == 0, preventing an
out-of-bounds read on vstate->frame[-1] for async callbacks queued via
push_async_cb().

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5601d20562186..51e07d1aaea4f 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) {
> +		verbose(env, "stack arg read with no caller frame\n");
> +		return -EINVAL;
> +	}
> +
>  	caller = vstate->frame[vstate->curframe - 1];
>  	arg = &caller->stack_arg_regs[spi];

[Severity: Critical]
Is checking vstate->curframe == 0 sufficient to ensure that
caller->stack_arg_regs[spi] is valid and initialized here?

Synchronous callbacks (such as those passed to bpf_loop or
bpf_for_each_map_elem) are queued via push_callback_call(), which advances
curframe via push_stack() and setup_func_entry() so that curframe >= 1 and
state->in_callback_fn is true:

kernel/bpf/verifier.c:push_callback_call() {
	caller = state->frame[state->curframe];
	err = btf_check_subprog_call(env, subprog);
	if (err == -EFAULT)
		return err;
	...
	callback_state = push_stack(env, env->subprog_info[subprog].start, insn_idx, false);
	if (IS_ERR(callback_state))
		return PTR_ERR(callback_state);

	err = setup_func_entry(env, subprog, insn_idx, set_callee_state_cb,
			       callback_state);
}

When a static subprogram declares more than 5 BTF arguments,
prepare_subprog_args() caches sub->arg_slot_cnt > 5 (so
bpf_in_stack_arg_cnt(subprog) > 0), and btf_check_func_arg_match() can return
-EINVAL before reaching check_func_args() -> check_outgoing_stack_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);
}

Similarly, once a static subprogram is marked unreliable by a prior call,
btf_check_subprog_call() returns -EINVAL immediately without running
check_outgoing_stack_args() for the new caller frame:

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;
}

Because push_callback_call(), check_func_call(), and check_func_callx()
ignore -EINVAL from btf_check_subprog_call(), execution can reach
check_stack_arg_read() with vstate->curframe >= 1 without
check_outgoing_stack_args() having validated the caller's outgoing stack
arguments.

If caller->out_stack_arg_cnt <= spi, won't indexing caller->stack_arg_regs[spi]
trigger a NULL pointer dereference or slab out-of-bounds read? And for a
synchronous callback where the frame invoking the helper wrote an outgoing
stack argument slot prior to the helper call, push_callback_call() clones
caller->stack_arg_regs before invalidate_outgoing_stack_args() runs, so
wouldn't check_stack_arg_read() copy that register state into
cur->regs[dst_regno] even though the kernel helper does not pass BPF stack
arguments at runtime?

Should check_stack_arg_read() also reject loads when state->in_callback_fn is
set, or when spi >= caller->out_stack_arg_cnt or
caller->stack_arg_regs[spi].type == NOT_INIT?

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