* Re: [PATCH bpf-next v2 22/23] bpf, arm64: Add JIT support for stack arguments
[not found] ` <20260509021523.D4469C2BCB0@smtp.kernel.org>
@ 2026-05-10 17:32 ` Yonghong Song
0 siblings, 0 replies; only message in thread
From: Yonghong Song @ 2026-05-10 17:32 UTC (permalink / raw)
To: sashiko; +Cc: bpf
On 5/9/26 4:15 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] In `emit_stack_arg_load()`, `bpf_off` is not negated, causing a negative index that fails array bounds checks and results in the JIT embedding an invalid instruction (BRK), leading to a kernel panic.
> --
>
> commit 6ccf042549a3902757c6335313c905e99fb41292
> Author: Puranjay Mohan <puranjay@kernel.org>
>
> bpf, arm64: Add JIT support for stack arguments
>
> This commit implements stack argument passing for BPF-to-BPF and kfunc
> calls with more than 5 parameters on arm64, following the AAPCS64 calling
> convention.
>
>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>> index 085e650662e3d..9b9d2501d2d36 100644
>> --- a/arch/arm64/net/bpf_jit_comp.c
>> +++ b/arch/arm64/net/bpf_jit_comp.c
> [ ... ]
>
>> @@ -1191,6 +1207,41 @@ static int add_exception_handler(const struct bpf_insn *insn,
>> return 0;
>> }
>>
>> +static const u8 stack_arg_reg[] = { A64_R(5), A64_R(6), A64_R(7) };
>> +
>> +#define NR_STACK_ARG_REGS ARRAY_SIZE(stack_arg_reg)
>> +
>> +static void emit_stack_arg_load(u8 dst, s16 bpf_off, struct jit_ctx *ctx)
>> +{
>> + int idx = bpf_off / sizeof(u64) - 1;
> Does this missing negation for bpf_off lead to an invalid instruction being
> emitted?
For stack argument load, the offset is positive number and it is multiple of 8.
So it should be okay here.
>
> Since BPF stack arguments use negative offsets from BPF_REG_PARAMS (such as
> -8 for arg 6), the calculation bpf_off / sizeof(u64) - 1 will result in a
> negative idx.
>
> Because NR_STACK_ARG_REGS uses ARRAY_SIZE, it evaluates to an unsigned
> size_t. Comparing a negative idx to it will result in a false condition,
> falling through to the else block.
>
> In the else block, the negative idx generates a negative offset for
> A64_LDR64I. Since A64_LDR64I expects an unsigned 12-bit scaled immediate,
> could this cause the instruction encoder to fail and embed an
> AARCH64_BREAK_FAULT into the JIT image?
>
>> +
>> + if (idx < NR_STACK_ARG_REGS)
>> + emit(A64_MOV(1, dst, stack_arg_reg[idx]), ctx);
>> + else
>> + emit(A64_LDR64I(dst, A64_FP, (idx - NR_STACK_ARG_REGS) * sizeof(u64) + 16), ctx);
>> +}
>> +
>> +static void emit_stack_arg_store(u8 src_a64, s16 bpf_off, struct jit_ctx *ctx)
>> +{
>> + int idx = -bpf_off / sizeof(u64) - 1;
> For comparison, emit_stack_arg_store() correctly negates bpf_off here.
>
>> +
>> + if (idx < NR_STACK_ARG_REGS)
^ permalink raw reply [flat|nested] only message in thread