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

[Yonghong Song <yonghong.song@linux.dev>]

On 10/8/26 10:14 AM, sashiko-bot@kernel.org wrote:
> 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.

In the above, we have aux->func[subprog]->aux->might_unwind. If it is true,
that means bpf_throw() will be rejected. So I think we should be okay.

>
> 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.

The issue here seems more appropriate for commit
"bpf: Dispatch cleanup pads by rewriting return addresses". I didn't
find any issues. Selftest results match expectations (manually checked).