Re: [PATCH v2] 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:
- [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