Re: [PATCH v4 bpf-next 2/6] bpf, x86: Support unwind_goto and unwind_exit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf, x86: emit_unwind_ret_addr() in exception_cb unwinds caller frame
--

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

bpf, x86: Support unwind_goto and unwind_exit

This commit adds x86 JIT support for the BPF unwind_goto and unwind_exit
instructions using exception table entries and return address fixups.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 72a2a0a89e033..d3ab36281144d 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
[ ... ]
> +static u64 bpf_unwind_target(u64 ret_addr)
> +{
> +	const struct exception_table_entry *e = search_bpf_extables(ret_addr - 1);
> +
> +	/*
> +	 * The verifier made sure that unwind_goto follows a call in a prog. When
> +	 * the kernel called an async or an exception callback, it's exit.
> +	 */
> +	return e ? ret_addr + e->fixup : ret_addr;
> +}
> +
> +/* unwind_exit: fix the return address of this frame. r0 and r2 are returned. */
> +static int emit_unwind_ret_addr(u8 **pprog, u8 *ip)
> +{
> +	u8 *prog = *pprog, *start = prog;
> +
> +	EMIT2(0x50, 0x56);			/* push rax; push rsi */
> +	EMIT4(0x48, 0x8B, 0x7D, 0x08);		/* mov rdi, [rbp + 8] */
> +	ip += prog - start;
> +	ip += x86_call_depth_emit_accounting(&prog, bpf_unwind_target, ip);
> +	if (emit_call(&prog, bpf_unwind_target, ip))
> +		return -EINVAL;
> +	EMIT4(0x48, 0x89, 0x45, 0x08);		/* mov [rbp + 8], rax */
> +	EMIT2(0x5E, 0x58);			/* pop rsi; pop rax */
> +	*pprog = prog;
> +	return 0;
> +}
[ ... ]
> @@ -3774,13 +3865,16 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>  			break;
>  
>  		case BPF_JMP | BPF_EXIT:
> +			if (src_reg == BPF_UNWIND_EXIT && bpf_prog_can_unwind(bpf_prog) &&
> +			    emit_unwind_ret_addr(&prog, ip))
> +				return -EINVAL;

[Severity: High]
Can emitting emit_unwind_ret_addr() inside an exception callback subprogram
cause unwind_exit to unwind the exception boundary's caller frame into an
unverified landing pad?

In include/linux/filter.h, bpf_prog_can_unwind() returns true for any
subprogram:

include/linux/filter.h:bpf_prog_can_unwind() {
	return bpf_is_subprog(prog) || prog->type == BPF_PROG_TYPE_EXT;
}

Because an exception callback is a subprogram (bpf_prog->aux->func_idx != 0
with bpf_prog->aux->exception_cb set), bpf_prog_can_unwind(bpf_prog) is true
in do_jit(), while the verifier treats BPF_UNWIND_EXIT in an exception
callback as a normal frame-0 exit in process_bpf_exit_full() and ignores the
exception callback when setting prog->aux->can_unwind in check_unwind_insns().

The comment in bpf_unwind_target() assumes that when the kernel invokes an
exception callback, the return address at [rbp + 8] belongs to the kernel
caller and will not match any BPF extable entry. However, in
arch/x86/net/bpf_jit_comp.c, emit_prologue() resets rbp to the third argument
(ctx.bp of the exception boundary frame found by bpf_stack_walker()):

arch/x86/net/bpf_jit_comp.c:emit_prologue() {
	if (is_exception_cb) {
		EMIT3(0x48, 0x89, 0xF4); /* mov rsp, rsi */
		EMIT3(0x48, 0x89, 0xD5); /* mov rbp, rdx */
		...
		EMIT3(0x48, 0x89, 0xEC); /* mov rsp, rbp */
}

As a result, [rbp + 8] inside the exception callback is the return address of
the exception boundary frame (func[0]), not bpf_throw().

If the exception boundary frame was entered via a tail call from a subprogram
that was called before an unwind_goto instruction, or if an extension program
(BPF_PROG_TYPE_EXT) with can_unwind == false on func[0] replaces a global
subprogram followed by an unwind_goto instruction and then calls bpf_throw(),
[rbp + 8] points to the instruction after the subprogram call in the caller.
When the exception callback executes BPF_UNWIND_EXIT, bpf_unwind_target()
finds the caller's unwind_goto extable entry and redirects [rbp + 8] to the
landing pad with register and stack state that the verifier never checked for
that path.

Should bpf_prog_can_unwind() (or the check in do_jit()) exclude
prog->aux->exception_cb?

[ ... ]

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