Re: [PATCH bpf-next v12 07/25] bpf: Refuse a callback that can unwind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: missing might_unwind propagation to callback in bpf_check_cfg()
--

commit 1785732c5b5ae17b86f08f6a94449da36fb9c958
Author: Yonghong Song <yonghong.song@linux.dev>

bpf: Refuse a callback that can unwind

This commit adds bpf_exc_check_callback() and calls it in
push_callback_call() to reject callback subprograms that have
might_unwind set.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fce2baa6272d1..fcf0d0fe0f2ff 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11191,6 +11191,10 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
>  	 * callbacks
>  	 */
>  	env->subprog_info[subprog].is_cb = true;
> +	err = bpf_exc_check_callback(env, subprog);
> +	if (err)
> +		return err;
> +
>  	if (bpf_pseudo_kfunc_call(insn) &&
>  	    !is_callback_calling_kfunc(insn->imm)) {
>  		verifier_bug(env, "kfunc %s#%d not marked as callback-calling",

[Severity: High]
Can a callback subprogram that transitively calls bpf_unwind() bypass
bpf_exc_check_callback() with env->subprog_info[subprog].might_unwind
still false?

In bpf_check_cfg(), direct calls (BPF_PSEUDO_CALL), function address
loads (BPF_PSEUDO_FUNC), and read-only map function pointer loads
(insn_func_ptrs()) are explored in a single DFS sharing
env->cfg.insn_state[]:

kernel/bpf/cfg.c:merge_callee_effects() {
	caller = bpf_find_containing_subprog(env, t);
	callee = bpf_find_containing_subprog(env, w);
	caller->changes_pkt_data |= callee->changes_pkt_data;
	caller->might_sleep |= callee->might_sleep;
	caller->might_throw |= callee->might_throw;
	caller->might_unwind |= callee->might_unwind;
}

merge_callee_effects() relies on callee w already being EXPLORED when
returning to caller t. However, if subprogram H has a conditional branch
whose fallthrough loads a read-only map containing the address of
callback cb via insn_func_ptrs() (or loads cb via BPF_PSEUDO_FUNC when H
is explored from a cleanup pad before main's fallthrough) and whose jump
target calls bpf_unwind(), while cb calls H via BPF_PSEUDO_CALL:

1. bpf_check_cfg() enters H (marking insn_state[w_H] = DISCOVERED) and
   explores H's fallthrough first, before visiting H's bpf_unwind()
   branch, so H->might_unwind is still false.

2. H's fallthrough visits cb via visit_func_ptrs_insn() or
   visit_func_call_insn().

3. When cb visits its BPF_PSEUDO_CALL to H, merge_callee_effects() sees
   H->might_unwind == false, and push_insn() sees
   (insn_state[w_H] & 0xF0) == DISCOVERED and returns DONE_EXPLORING.
   Thus cb finishes CFG exploration with cb->might_unwind == false
   before H explores its bpf_unwind() branch and sets
   H->might_unwind = true.

4. At the end of bpf_check_cfg(), mark_callx_might_unwind() checks
   addr_taken_subprog_might_unwind():

kernel/bpf/cfg.c:mark_callx_might_unwind() {
	if (!addr_taken_subprog_might_unwind(env))
		return;
    ...
}

   Because the address-taken subprogram cb still has might_unwind == false,
   addr_taken_subprog_might_unwind() returns false and skips the
   fixed-point propagation loop.

5. Neither sort_subprogs_topo() nor check_max_stack_depth() rejects the
   program because insn_func_ptrs() edges are not call edges and async
   BPF_PSEUDO_FUNC callbacks are skipped in stack depth checks.

When push_callback_call() then calls bpf_exc_check_callback(env, cb),
env->subprog_info[cb].might_unwind is false, allowing the unwinding
callback to pass verification and cause a verifier/runtime control-flow
mismatch or jump to a zero epilogue_ip when bpf_unwind() is invoked at
runtime.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017025129.1914859-1-yonghong.song@linux.dev?part=7