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

[sashiko-bot@kernel.org]

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