Re: [PATCH bpf-next v9 12/23] bpf: Refuse a trampoline that calls a subprog that can unwind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: x86 exception unwinder passes wrong return address pointer causing stack corruption
- [High] bpf: Missing trampoline restriction for bpf_throw() leads to NULL pointer deref
--

commit 554b5300d0510c78440f3ee71cad1d363c75cfaf
Author: Yonghong Song <yonghong.song@linux.dev>

bpf: Refuse a trampoline that calls a subprog that can unwind

This patch refuses fexit, fmod_ret, and fsession trampoline attachments
to subprograms marked with might_unwind, preventing bpf_unwind() from
tripping over unexpected trampoline frames.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 668d811d4e4cb..3692d9b163d43 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -21600,6 +21600,22 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  				prog_extension ? "Extension" : "Tracing");
>  			return -EINVAL;
>  		}
> +		/*
> +		 * A trampoline that calls its target stays as a frame between a
> +		 * subprog and its caller, and bpf_unwind() cannot walk past it:
> +		 * the frames below would return after their calls rather than at
> +		 * their landing pads. fentry leaves no frame, and the main
> +		 * program's caller is below where the walk stops.
> +		 */
> +		if (aux->func && subprog && aux->func[subprog]->aux->might_unwind &&
> +		    (prog->expected_attach_type == BPF_TRACE_FEXIT ||
> +		     prog->expected_attach_type == BPF_MODIFY_RETURN ||
> +		     prog->expected_attach_type == BPF_TRACE_FSESSION)) {

[Severity: High]
Does this logic also need to check for bpf_throw()?

The patch correctly prevents attachments for bpf_unwind(), which is tracked
by might_unwind. However, bpf_throw() seems to be missed. It uses an
independent might_throw flag which does not appear to be propagated to
bpf_prog_aux or checked here.

If a subprogram calls bpf_throw() and has a tracing program attached
(like fexit), bpf_stack_walker() encounters the trampoline frame. At that
point, bpf_prog_ksym_find() will return NULL, and since ctx->cnt > 0, it aborts
the walk before setting ctx->aux:

kernel/bpf/helpers.c:bpf_stack_walker() {
    ...
    if (!prog)
        return !ctx->cnt;
    ...
}

Because the walk is aborted early, ctx.aux remains NULL, which leads to a
kernel panic when dereferenced later in bpf_throw():

kernel/bpf/helpers.c:bpf_throw() {
    ...
    ctx.aux->bpf_exception_cb(cookie, ctx.sp + ctx.aux->stack_arg_sp_adjust,
                              ctx.bp, 0, 0);
    ...
}

> +			bpf_log(log,
> +				"Cannot attach fexit, fmod_ret or fsession to %s, which can unwind\n",
> +				tname);
> +			return -EINVAL;
> +		}
>  		conservative = aux->func_info_aux[subprog].unreliable;
>  		if (prog_extension) {

[Severity: Critical]
I also have a question about the x86 exception unwinding implementation added
earlier in this series. Can the x86 unwinder corrupt the stack by passing the
wrong return address pointer?

In arch_bpf_stack_walk_ra(), the unwinder passes the current frame's return
address pointer to the callback:

arch/x86/net/bpf_jit_comp.c:arch_bpf_stack_walk_ra() {
    ...
    ra = unwind_get_return_address_ptr(&state);
    if (!ra)
        break;
    if (!consume_fn(cookie, (u64)addr, (u64)state.sp, (u64)state.bp, (u64 *)ra))
    ...
}

However, bpf_unwind_rewrite() seems to expect the return address pointer
of the previous frame, so it can make the previous frame return directly into
the current frame's cleanup pad.

By passing the current frame's return address instead, bpf_unwind_rewrite()
overwrites the current frame's return address with its own pad or epilogue:

kernel/bpf/helpers.c:bpf_unwind_rewrite() {
    ...
    rec = bpf_exc_pad_for_ip(prog, ip);
    *ra = rec ? rec->pad : prog->aux->epilogue_ip;
    ...
}

On x86, this causes the pad to execute after the current frame has already
returned via leave and ret, corrupting the caller's stack frame. For the main
program, its return address is overwritten with its own epilogue, leading to
a double-epilogue execution.

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