Re: [RFC PATCH bpf-next v1 05/11] bpf: Let the callback of bpf_loop unwind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: inline_bpf_loop() emits dangling jump to dead unwind pad
- [Medium] bpf: clobbering of caller R2 in prepare_func_exit() on callback unwind
--

commit 342597e9c392b90ccb12e07eaf21a9e820d7f364
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Let the callback of bpf_loop unwind

This commit adds an unwind landing pad to bpf_loop() using bpf_call_cb() and
bpf_unwind_leave(), and updates helper_has_cb_pad() so the verifier permits
bpf_loop() callbacks to unwind.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ac9dbe0821811..4b4d8479fa87b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11402,10 +11402,15 @@ static int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog)
>  	return err;
>  }
>  
> -/* The helper calls its callback with a pad, see bpf_call_cb(). None yet. */
> +/* The helper calls its callback with a pad, see bpf_call_cb() */
>  static bool helper_has_cb_pad(enum bpf_func_id func_id)
>  {
> -	return false;
> +	switch (func_id) {
> +	case BPF_FUNC_loop:
> +		return true;
> +	default:
> +		return false;
> +	}
>  }

[Severity: Critical]
When helper_has_cb_pad(BPF_FUNC_loop) returns true, can inline_bpf_loop() in
kernel/bpf/fixups.c emit a live BPF_JMP32_A instruction that jumps to a dead
landing pad which is subsequently removed by dead code elimination, or leave a
stray BPF_UNWIND_GOTO after a load instruction?

In kernel/bpf/verifier.c:check_unwind_insns(), subprog_info[subprog].can_unwind
is set statically whenever the callback contains an unwind_exit instruction,
and check_cb_unwind() then requires an unwind_goto instruction at position + 1
immediately after the bpf_loop() call.

If a privileged program calls bpf_loop() with flags == 0 and a single callback
target (so fit_for_inline is true), and the callback does not dynamically take
its unwind_exit path during verification of that call (for example, when
guarded by callback_ctx or loop index, or when nr_loops == 0), state->unwound
is never set at position + 1. In check_cond_jmp_op(), the unwind_goto branch
to the caller's landing pad (caller_pad = position + 2 + unwind_goto->off) is
never taken, leaving env->insn_aux_data[caller_pad].seen == 0.

In kernel/bpf/verifier.c:bpf_check(), bpf_optimize_bpf_loop() runs before dead
code elimination:

	if (ret == 0)
		ret = bpf_optimize_bpf_loop(env);

	if (is_priv) {
		if (ret == 0)
			bpf_opt_hard_wire_dead_code_branches(env);
		if (ret == 0)
			ret = bpf_opt_remove_dead_code(env);

Inside kernel/bpf/fixups.c:inline_bpf_loop(), pad is computed without checking
whether the target of unwind_goto at position + 1 was actually visited:

	bool pad = env->subprog_info[callback_subprogno].can_unwind &&
		   bpf_is_unwind_goto(&env->prog->insnsi[position + 1]);
	...
	if (pad) {
		struct bpf_insn *unwind_goto = &new_prog->insnsi[position + cnt];

		/* the jump of the pad, one insn before, takes the target of the unwind_goto */
		new_prog->insnsi[position + cnt - 1].imm = unwind_goto->off + 1;
		/* and the unwind_goto after the call of bpf_loop follows no call anymore */
		*unwind_goto = BPF_JMP_A(0);
	}

Because adjust_insn_aux_data() copies the call instruction's non-zero seen
count to all patched instructions, the inlined pad and its trailing
BPF_JMP32_A are marked seen != 0. bpf_opt_hard_wire_dead_code_branches() does
not fold the inlined BPF_UNWIND_GOTO (its target is the inlined pad, which has
seen != 0) and ignores the unconditional BPF_JMP32_A.

When bpf_opt_remove_dead_code() then deletes the unvisited caller_pad via
verifier_remove_insns() -> bpf_remove_insns() -> bpf_adj_branches() ->
bpf_adj_delta_to_off() in kernel/bpf/core.c:

	if (curr < pos && curr + off + 1 >= end_old)
		off += delta;
	else if (curr >= end_new && curr + off + 1 < end_new)
		off -= delta;

BPF_JMP32_A's offset is not adjusted because its target falls inside the
removed range [pos, end_old). If caller_pad was at the end of the subprogram,
func[i]->len shrinks while BPF_JMP32_A still jumps past the end of the
subprogram, causing out-of-bounds reads of addrs[i + insn->imm] in do_jit() in
arch/x86/net/bpf_jit_comp.c and ctx->offset[bpf_insn + off] in
bpf2a64_offset() in arch/arm64/net/bpf_jit_comp.c, or jumping into post-return
int3 padding or subsequent instructions.

Additionally, if the callback has can_unwind == false and an unwind_goto
follows the bpf_loop() call (which check_unwind_insns() and check_cb_unwind()
allow), pad is false in inline_bpf_loop(), so *unwind_goto = BPF_JMP_A(0) is
skipped, leaving BPF_UNWIND_GOTO immediately after BPF_LDX_MEM instead of a
call instruction when the landing pad is shared (seen != 0) or !is_priv.

[Severity: Medium]
What happens in prepare_func_exit() in kernel/bpf/verifier.c when a bpf_loop()
callback subprogram with a 9 to 16 byte BTF return type (ret_reg_pair == true)
unwinds via unwind_exit?

In kernel/bpf/verifier.c:bpf_compute_subprog_ret_regs(), any static subprogram
whose BTF return type has size in [9, 16] bytes gets ret_reg_pair = true, and
neither check_func_args() nor btf_check_subprog_call() rejects such a
subprogram when passed as ARG_PTR_TO_FUNC.

With helper_has_cb_pad(BPF_FUNC_loop) now returning true, the
callee->in_callback_fn && unwinds path in prepare_func_exit() becomes
reachable:

	callee = state->frame[state->curframe];
	r0 = &callee->regs[BPF_REG_0];
	nregs = bpf_ret_reg_pair(env, callee->subprogno) ? 2 : 1;
	for (i = 0; i < nregs; i++) {
		if (callee->regs[ret_regs[i]].type != PTR_TO_STACK)
			continue;
		verbose(env, "cannot return stack pointer to the caller\n");
		return -EINVAL;
	}
	...
		for (i = 0; i < nregs; i++) {
			u32 regno = ret_regs[i];

			bpf_diag_mod_begin(env, &caller->regs[regno], &callee->regs[regno],
					   BPF_DIAG_MOD_WRITE);
			caller->regs[regno] = callee->regs[regno];
			bpf_diag_mod_end(env);
		}

Even though bpf_call_cb() in bpf_loop() only propagates R0 to the landing pad,
prepare_func_exit() uses nregs = 2 (ret_regs = { BPF_REG_0, BPF_REG_2 })
without checking !callee->in_callback_fn.

First, because set_loop_callback_state() initializes the callback's R2 to
caller->regs[BPF_REG_3] (callback_ctx), passing a stack pointer as
callback_ctx causes the PTR_TO_STACK check on callee->regs[BPF_REG_2] to
falsely reject the unwind if the callback did not overwrite R2.

Second, prepare_func_exit() copies callee->regs[BPF_REG_2] into
caller->regs[BPF_REG_2] before setting *insn_idx = callee->callsite to revisit
the bpf_loop() call instruction. When check_helper_call() re-runs with
env->cur_state->unwound == true, check_func_args() re-validates
caller->regs[BPF_REG_1..5] before caller-saved registers are scrubbed. Because
caller->regs[BPF_REG_2] (the callback_fn argument) was overwritten with the
callback's R2, could check_func_args() fail verification or update
meta.subprogno in update_loop_inline_state() to a different subprogram?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=5