> Fixes: d243b62b7bd3 ("powerpc64/bpf: Add support for bpf trampolines")
Does the Fixes: tag name the correct commit?
With Cc: [email protected] this fix will only reach v6.13+ and will
leave every stable tree from v5.18 to v6.12 carrying the same JIT buffer
overflow.
The faulty logic predates d243b62b7bd3. Looking at the parent of that commit:
git show d243b62b7bd3^:arch/powerpc/net/bpf_jit_comp.c
already contains the exact code being fixed:
if (!exit_addr || is_offset_in_branch_range(exit_addr - (ctx->idx * 4))) {
PPC_JMP(exit_addr);
} else if (ctx->alt_exit_addr) {
if (WARN_ON(!is_offset_in_branch_range((long)ctx->alt_exit_addr -
(ctx->idx * 4))))
return -1;
PPC_JMP(ctx->alt_exit_addr);
} else {
ctx->alt_exit_addr = ctx->idx * 4;
bpf_jit_build_epilogue(image, ctx);
}
The real introducing commit is 0ffdbce6f4a89bb7c0002904d6438ec83cf05ce7
("powerpc/bpf: Handle large branch ranges with BPF_EXIT"), merged in v5.18-rc1
(git describe --contains: v5.18-rc1~124^2~42). It created
bpf_jit_emit_exit_insn() with the '!exit_addr' early-out and replaced the
previous unconditional one-instruction emission:
- if (i != flen - 1)
- PPC_JMP(exit_addr);
+ if (i != flen - 1) {
+ ret = bpf_jit_emit_exit_insn(image, ctx,
b2p[TMP_REG_1], exit_addr);
Before 0ffdbce6f4a8 a non-final BPF_EXIT always cost exactly one instruction in
every pass, so the sizing pass could not under-count and no overflow was
possible. After it, the sizing pass emits PPC_JMP(0) (1 instruction) while
later passes may emit a full inline epilogue, which is precisely the
under-count described in this commit message.
The commit d243b62b7bd3 only enlarged the epilogue (it added
bpf_jit_build_fentry_stubs()), making an already-existing overflow bigger.
Citing the ppc64-trampoline commit also wrongly implies PPC32 is unaffected.
The commit 0ffdbce6f4a8 made the identical change to
arch/powerpc/net/bpf_jit_comp32.c, so 32-bit powerpc has the same bug over the
same range.
A tag survey confirms the range: 'alt_exit_addr' has 0 occurrences in
arch/powerpc/net/ at v5.16 and v5.17, and 5 occurrences at v5.18, v5.19, v6.0,
v6.1 and v6.6.
Should the tag be:
Fixes: 0ffdbce6f4a8 ("powerpc/bpf: Handle large branch ranges with BPF_EXIT")
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 8f7501954d9f0..11981d2270a9d 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -99,11 +99,10 @@ void bpf_jit_build_fentry_stubs(u32 *image, struct
> codegen_context *ctx)
>
> int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int
> tmp_reg, long exit_addr)
> {
> - if (!exit_addr || is_offset_in_branch_range(exit_addr - (ctx->idx *
> 4))) {
> + if (exit_addr && is_offset_in_branch_range(exit_addr - (long)(ctx->idx
> * 4))) {
> PPC_JMP(exit_addr);
> - } else if (ctx->alt_exit_addr) {
> - if (WARN_ON(!is_offset_in_branch_range((long)ctx->alt_exit_addr
> - (ctx->idx * 4))))
> - return -1;
> + } else if (ctx->alt_exit_addr && is_offset_in_branch_range(
> + (long)(ctx->alt_exit_addr) - (long)(ctx->idx * 4))) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> PPC_JMP(ctx->alt_exit_addr);
With alt_exit_addr defined as unsigned int, does the expression
(long)(ctx->alt_exit_addr) - (long)(ctx->idx * 4) produce a different result
than (long)ctx->alt_exit_addr - (ctx->idx * 4) due to how integer promotion
works?
In v7 review, Hari Bathini pointed out that the cast placement may affect
integer promotion semantics and requested restoring the earlier syntax with the
cast only on alt_exit_addr:
https://lore.kernel.org/bpf/[email protected]
While the current code has passed testing, the concern about integer promotion
semantics was never explicitly addressed.
[ ... ]
> @@ -399,7 +417,7 @@ int bpf_add_extable_entry(struct bpf_prog *fp, u32
> *image, u32 *fimage, int pass
> u32 *fixup;
>
> /* Populate extable entries only in the last pass */
> - if (pass != 2)
> + if (pass < CODEGEN_MIN_PASSES)
> return 0;
Does the comment still match the code?
With the old 'if (pass != 2)' and a fixed two-pass loop, pass 2 was the last
pass, so the comment was accurate. With the new loop and 'pass <
CODEGEN_MIN_PASSES', both pass 2 and pass 3 can write extable entries, so
entries are populated in every pass from 2 onwards, not only in the last one.
Also, CODEGEN_MIN_PASSES is being reused here with a different meaning. In
arch/powerpc/net/bpf_jit.h it documents the minimum number of code-generation
passes, but here it is used as 'the first pass in which extable entries are
valid'. The two are logically independent - bumping CODEGEN_MIN_PASSES to 3 to
force more settling iterations would silently stop extable entries being
written in pass 2.
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33368376983
