Re: [PATCH bpf-next v2 8/9] bpf, x86: Fix stack-passed arguments for indirect trampolines
"Kumar Kartikeya Dwivedi" <[email protected]>
| Newsgroups | dev.linux.lists.sashiko-reviews,org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
On Sun Jul 26, 2026 at 3:51 AM CEST, sashiko-bot wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > > Pre-existing issues: > - [High] The patch fixes the stack-passed argument offset for indirect struct_ops trampolines on x86, but leaves the exact same logical bug unfixed in the ARM64, RISC-V, and LoongArch BPF JIT compilers. > -- > > --- Patch [8]: [PATCH bpf-next v2 8/9] bpf, x86: Fix stack-passed arguments for indirect trampolines --- > commit 1625042935fc87b38acf83c8304433545d672f35 > Author: Tejun Heo <[email protected]> > > bpf, x86: Fix stack-passed arguments for indirect trampolines > > This patch fixes the calculation of stack-passed arguments for indirect > trampolines in the x86 BPF JIT compiler. It introduces stack_args_off > to properly handle the number of return addresses on the stack depending > on how the trampoline was called. > >> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c >> index dfb5335ad8374..4b349ae35ebf9 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: High] > This is a pre-existing issue, but does this same logical bug need to be fixed > for ARM64, RISC-V, and LoongArch? > > Looking at arch/arm64/net/bpf_jit_comp.c:save_args(), there is a hardcoded > offset that unconditionally assumes two frames were pushed: > > soff = 32; /* on stack arguments start from FP + 32 */ > > Similarly, in arch/riscv/net/bpf_jit_comp64.c:store_args(), there is a > hardcoded offset of 16 which skips the first 16 bytes of caller arguments for > struct_ops: > > emit_ld(RV_REG_T1, 16 + (i - RV_MAX_REG_ARGS) * 8, RV_REG_FP, ctx); > > And arch/loongarch/net/bpf_jit.c:store_args() shares the same hardcoded offset: > > emit_insn(ctx, ldd, LOONGARCH_GPR_T1, LOONGARCH_GPR_FP, > 16 + (i - LOONGARCH_MAX_REG_ARGS) * 8); > > If a struct_ops callback with more than 8 arguments (or 6 on some > architectures) is executed on these platforms, could it receive out-of-bounds > stack memory instead of the actual arguments? > Will be followed up on separately for each arch. >> int i, j, slot = 0; >> >> /* Store function arguments to stack. > [ ... ]