Re: [PATCH bpf-next v3 8/9] bpf, x86: Fix stack-passed arguments for indirect trampolines

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The fix for stack-passed arguments in indirect trampolines was only applied to x86, leaving identical bugs in ARM64, RISC-V, and LoongArch.
--

--- Patch [8]: [PATCH bpf-next v3 8/9] bpf, x86: Fix stack-passed arguments for indirect trampolines ---
commit 010f92af9c8f5bd1141eaf8fc752e0f9df18e07d
Author: Tejun Heo <tj@kernel.org>

bpf, x86: Fix stack-passed arguments for indirect trampolines

This commit fixes the stack offset for arguments passed on the stack
when entering an indirect trampoline on x86. The adjustment accounts for
the fact that only the caller's return address is on the stack, not the
origin function's return address.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index f70689b27845f..4b3e5d8a4a563 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -3075,6 +3075,7 @@ static void save_args(const struct btf_func_model *m, u8 **prog,
>  {
>  	int arg_regs, first_off = 0, nr_regs = 0, nr_stack_slots = 0;
>  	bool use_jmp = bpf_trampoline_use_jmp(flags);
> +	int stack_args_off = (use_jmp || (flags & BPF_TRAMP_F_INDIRECT)) ? 16 : 24;

[Severity: Medium]
This is a pre-existing issue, but since we are fixing the stack-passed
arguments for indirect trampolines on x86 here, should we also address
the same issue on other architectures?

For instance, on ARM64, in save_args() in arch/arm64/net/bpf_jit_comp.c,
the offset is hardcoded assuming both parent and patched function frames
were pushed:

    soff = 32; /* on stack arguments start from FP + 32 */

For indirect trampolines, only one frame is pushed, so the arguments would
start at FP + 16.

Similarly, in RISC-V's store_args() in arch/riscv/net/bpf_jit_comp64.c:

    emit_ld(RV_REG_T1, 16 + (i - RV_MAX_REG_ARGS) * 8, RV_REG_FP, ctx);

And in LoongArch's store_args() in arch/loongarch/net/bpf_jit.c:

    emit_insn(ctx, ldd, LOONGARCH_GPR_T1, LOONGARCH_GPR_FP,
              16 + (i - LOONGARCH_MAX_REG_ARGS) * 8);

Both of these hardcode the offset relative to FP, ignoring that FP is
aligned with SP for indirect trampolines. 

If a struct_ops callback is invoked with enough arguments to force
stack-passing on these architectures, will they read garbage values instead
of the actual arguments?

>  	int i, j, slot = 0;
>  
>  	/* Store function arguments to stack.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803125115.2264733-1-memxor@gmail.com?part=8