Re: [PATCH bpf-next v5 11/14] bpf, x86: Fix stack-passed arguments for indirect trampolines

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

Pre-existing issues:
- [High] Incomplete fix: The off-by-one-slot stack argument read bug for `BPF_TRAMP_F_INDIRECT` trampolines was fixed for x86 but identical bugs remain in arm64, riscv, and loongarch JITs.
--

--- Patch [11]: [PATCH bpf-next v5 11/14] bpf, x86: Fix stack-passed arguments for indirect trampolines ---
commit b424d38504eb2136da7da04d1aa98780ba7432d8
Author: Tejun Heo <[email protected]>
bpf, x86: Fix stack-passed arguments for indirect trampolines

This commit fixes the stack offset calculation for stack-passed arguments
in x86 indirect trampolines by accounting for the single return address
on the stack.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 162fbd2ba1df8..8dddb5d7af21b 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
[ ... ]
> @@ -3080,6 +3080,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: High]
This isn't a bug introduced by this patch, but does this same offset issue
need to be fixed in the trampolines for other architectures as well?

For example, in arm64, save_args() hardcodes an offset of 32:

arch/arm64/net/bpf_jit_comp.c:save_args() {
    ...
    soff = 32; /* on stack arguments start from FP + 32 */
    ...
}

This assumes two 16-byte frames are pushed. Since BPF_TRAMP_F_INDIRECT only
pushes one frame, reading from FP + 32 reads from the old SP + 16, skipping
16 bytes of arguments.

Similarly, riscv hardcodes a 16-byte offset relative to FP:

arch/riscv/net/bpf_jit_comp64.c:store_args() {
    ...
    emit_ld(RV_REG_T1, 16 + (i - RV_MAX_REG_ARGS) * 8, RV_REG_FP, ctx);
    ...
}

Since FP points directly to the old stack pointer for indirect trampolines,
this skips the first 16 bytes of caller arguments.

Loongarch has the identical pattern:

arch/loongarch/net/bpf_jit.c:store_args() {
    ...
    emit_insn(ctx, ldd, LOONGARCH_GPR_T1, LOONGARCH_GPR_FP,
              16 + (i - LOONGARCH_MAX_REG_ARGS) * 8);
    ...
}

If a struct_ops callback were to receive enough arguments to be passed on the
stack, would these architectures also pass garbage to the BPF program due to
reading past the actual start of the stack arguments?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11