Re: [PATCH v4 bpf-next 1/6] bpf: Add unwind_goto and unwind_exit insns

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: stale insn_aux_data and unvisited unwind_goto handling in bpf_opt_hard_wire_dead_code_branches()
- [Medium] bpf: missing jit_requested and offload checks in check_unwind_insns()
--

commit d6e6c3108f32eea3c296a97e0da89132803476f1
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Add unwind_goto and unwind_exit insns

This patch introduces the BPF_UNWIND_GOTO and BPF_UNWIND_EXIT instructions
to support stack frame unwinding in BPF programs without extra register
overhead or stack walking. It adds verifier tracking, dead-code branch
fixups, and disassembler and dump support for the new instructions.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 409b9104c723e..f062ef7dd943d 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -658,6 +658,7 @@ void bpf_opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
>  {
>  	struct bpf_insn_aux_data *aux_data = env->insn_aux_data;
>  	struct bpf_insn ja = BPF_JMP_IMM(BPF_JA, 0, 0, 0);
> +	struct bpf_insn trap = BPF_JMP_IMM(BPF_JA, 0, 0, -1);
>  	struct bpf_insn *insn = env->prog->insnsi;
>  	const int insn_cnt = env->prog->len;
>  	int i;
>  
> @@ -666,6 +667,20 @@ void bpf_opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
>  		if (!bpf_insn_is_cond_jump(insn->code))
>  			continue;
>  
> +		if (bpf_is_unwind_goto(insn) && aux_data[i + 1 + insn->off].seen) {
> +			/*
> +			 * The callee never exits. Its unwind_exit needs
> +			 * unwind_goto all the same, and the cpu predicts that it
> +			 * returns to what follows. Don't let the removal of dead
> +			 * code put something else there.
> +			 */
> +			if (!aux_data[i + 1].seen) {
> +				memcpy(insn + 1, &trap, sizeof(trap));
> +				aux_data[i + 1].seen = env->pass_cnt;
> +			}
> +			continue;
> +		}

[Severity: High]
When bpf_opt_hard_wire_dead_code_branches() overwrites insn + 1 with trap and
marks aux_data[i + 1].seen = env->pass_cnt, could stale flags in
aux_data[i + 1] corrupt the trap instruction or trigger a verifier bug?

Before do_check_main() and do_check_subprogs() run,
bpf_compute_live_registers() sets insn_aux[i + 1].zext_dst = true on the
static CFG if the original instruction at i + 1 defines a 32-bit subregister
whose upper half is live out:

kernel/bpf/liveness.c:bpf_compute_live_registers() {
	...
	for (i = 0; i < insn_cnt; ++i) {
		int def32 = bpf_insn_def32(env->prog, &insns[i]);
		u32 out = state[i].out;
		u32 in = state[i].in;

		insn_aux[i].live_regs_before = mask_lo(in) | mask_hi(in);
		insn_aux[i].zext_dst = def32 >= 0 && (mask_hi(out) & BIT(def32));
	}
}

In addition, when !env->bypass_spec_v1 is true, check_cond_jmp_op() calls
sanitize_speculative_path(env, NULL, *insn_idx + 1, *insn_idx) for
unwind_goto. That speculative walk leaves aux_data[i + 1].seen as 0 via
sanitize_mark_insn_seen(), while do_check_insn() can set zext_dst,
needs_zext, or arena_scalar on aux_data[i + 1].

Unlike keep_funcs_with_addr_taken() and sanitize_dead_code(), which clear
zext_dst = false when overwriting an unvisited instruction with trap,
bpf_opt_hard_wire_dead_code_branches() leaves aux_data[i + 1] flags intact.

Later, bpf_do_misc_fixups() will rewrite the BPF_JA trap instruction if
needs_zext or arena_scalar is set on aux_data[i + 1]:

kernel/bpf/fixups.c:bpf_do_misc_fixups() {
	...
		if (env->insn_aux_data[i + delta].needs_zext)
			/* Convert BPF_CLASS(insn->code) == BPF_ALU64 to 32-bit ALU */
			insn->code = BPF_ALU | BPF_OP(insn->code) | BPF_SRC(insn->code);

		if (env->insn_aux_data[i + delta].arena_scalar) {
			cnt = arena_scalar_access(insn, insn_buf);
	...
}

And on architectures where bpf_jit_needs_zext() is true,
bpf_opt_subreg_zext_lo32_rnd_hi32() sees load_reg == -1 on the BPF_JA trap
instruction while aux[adj_idx].zext_dst is set, hitting verifier_bug_if():

kernel/bpf/fixups.c:bpf_opt_subreg_zext_lo32_rnd_hi32() {
	...
		if (verifier_bug_if(load_reg == -1, env,
				    "zext_dst is set, but no reg is defined"))
			return -EFAULT;
}

Also, what happens if the call and unwind_goto at insn i are themselves in
dead code (!aux_data[i].seen), but jump to a shared landing pad that was
reached from another call (aux_data[i + 1 + insn->off].seen)? Because
aux_data[i].seen is not checked here, insn + 1 is overwritten with trap and
marked seen, so bpf_opt_remove_dead_code() removes the dead call and
unwind_goto while leaving an orphaned goto pc-1 in the program (and any
later live conditional jump with untaken target i + 1 can no longer see
!aux_data[i + 1].seen).

Should this check aux_data[i].seen before handling unwind_goto, and clear
zext_dst, needs_zext, and arena_scalar on aux_data[i + 1] when writing trap?

[ ... ]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 849bc9136299d..90bc7230afa8a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5087,6 +5087,12 @@ static struct bpf_insn *bpf_insn_prepare_dump(const struct bpf_prog *prog,
>  			insns[i].imm = BPF_FUNC_tail_call;
>  			/* fall-through */
>  		}
> +		/*
> +		 * BPF_JCOND is the same opcode as BPF_CALL_ARGS. The latter is for
> +		 * the interpreter, and a prog with unwind_goto is always JITed.
> +		 */
> +		if (prog->jited && bpf_is_unwind_goto(&insns[i]))
> +			continue;
>  		if (code == (BPF_JMP | BPF_CALL) ||
>  		    code == (BPF_JMP | BPF_CALL_ARGS)) {
>  			/* Restore the legacy xlated dump layout. */
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 01434e1d23b59..e387320e196f9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -3430,6 +3431,46 @@ static int next_callx_callee(struct bpf_verifier_env *env, int caller, int from)
>  	return bit < end ? bit - caller * cnt : -1;
>  }
>  
> +/*
> + * 'call foo; unwind_goto pad' goes to the pad when foo left with unwind_exit.
> + * JITs don't emit a branch for it. unwind_exit finds the pad by its return
> + * address. So unwind_goto has to follow the call and nothing may jump to it.
> + */
> +static int check_unwind_insns(struct bpf_verifier_env *env)
> +{
> +	struct bpf_insn *insn = env->prog->insnsi;
> +	bool seen = false;
> +	int i;
> +
> +	for (i = 0; i < env->prog->len; i++, insn++) {
> +		if (bpf_is_unwind_exit(insn)) {
> +			bpf_find_containing_subprog(env, i)->can_unwind = true;
> +			seen = true;
> +		}
> +		if (!bpf_is_unwind_goto(insn))
> +			continue;
> +		seen = true;
> +		if (!i || !(bpf_pseudo_call(insn - 1) || bpf_is_callx(insn - 1))) {
> +			verbose(env, "unwind_goto at insn %d doesn't follow a call of a subprog\n",
> +				i);
> +			return -EINVAL;
> +		}
> +		if (bpf_is_jump_target(env, i)) {
> +			verbose(env, "jump to unwind_goto at insn %d\n", i);
> +			return -EINVAL;
> +		}
> +	}
> +	if (!seen)
> +		return 0;
> +	if (!bpf_jit_supports_unwind()) {
> +		verbose(env, "JIT does not support unwinding\n");
> +		return -ENOTSUPP;
> +	}
> +	env->prog->jit_required = 1;
> +	env->prog->aux->can_unwind = env->subprog_info[0].can_unwind;
> +	return 0;
> +}

[Severity: Medium]
Should check_unwind_insns() also reject programs when
!env->prog->jit_requested or bpf_prog_is_offloaded(env->prog->aux)?

When CONFIG_BPF_JIT_ALWAYS_ON is enabled and a program with unwind_goto is
loaded with prog_ifindex set to an offload device (such as netdevsim),
bpf_fixup_call_args() skips bpf_jit_subprogs() because
!bpf_prog_is_offloaded(env->prog->aux) is false, and the
#ifndef CONFIG_BPF_JIT_ALWAYS_ON check on prog->jit_required is compiled out:

kernel/bpf/fixups.c:bpf_fixup_call_args() {
	...
	if (env->prog->jit_requested &&
	    !bpf_prog_is_offloaded(env->prog->aux)) {
		err = bpf_jit_subprogs(env);
		if (err == 0)
			return 0;
		if (err == -EFAULT || err == -EINTR)
			return err;
	}
#ifndef CONFIG_BPF_JIT_ALWAYS_ON
	if (prog->jit_required) {
		verbose(env, "program requires BPF JIT compiler but it is not available\n");
		return -EINVAL;
	}
	...
}

__bpf_prog_select_runtime() then takes the bpf_prog_offload_compile() branch
without checking jit_needed, leaving prog->jited as 0:

kernel/bpf/core.c:__bpf_prog_select_runtime() {
	...
	if (!bpf_prog_is_offloaded(fp->aux)) {
		...
		if (!fp->jited && jit_needed) {
			*err = -ENOTSUPP;
			return fp;
		}
	} else {
		*err = bpf_prog_offload_compile(fp);
		if (*err)
			return fp;
	}
}

Because prog->jited is 0 on that offloaded program, bpf_insn_prepare_dump()
skips the prog->jited && bpf_is_unwind_goto(&insns[i]) check and falls
through to the BPF_JMP | BPF_CALL_ARGS branch, where insns[i].src_reg ==
BPF_PSEUDO_CALL (both equal to 1) causes prepare_dump_pseudo_call() to
corrupt unwind_goto into BPF_JMP | BPF_CALL in xlated program dumps.

Similarly, when CONFIG_BPF_JIT_ALWAYS_ON is disabled and bpf_jit_enable is 0,
checking !env->prog->jit_requested in check_unwind_insns() (like
require_callx_jit() does) would reject unwinding before passes such as
bpf_compute_subprog_ret_regs() that gate on env->prog->jit_requested.

[ ... ]

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