Re: [PATCH v11 1/8] powerpc/bpf: fix alignment of long branch trampoline address

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dynamic epilogue sizing breaks JIT pass convergence, causing BPF_EXIT branches to skip the first instruction of the epilogue.
- [Low] The patch introduces a compilation error in `arch/powerpc/net/bpf_jit_comp32.c` by failing to remove the old 4-argument call to `bpf_jit_emit_exit_insn()`.

Pre-existing issues:
- [Critical] Out-of-bounds write in BPF JIT due to under-calculated allocation size for large programs.
--

--- Patch [1]: [PATCH v11 1/8] powerpc/bpf: fix alignment of long branch trampoline address ---
commit a15c5c8b15ef787442dd6c60c0e42163a958cb5a
Author: Abhishek Dubey <adubey@linux.ibm.com>

powerpc/bpf: fix alignment of long branch trampoline address

This patch ensures the dummy trampoline address field between the out-of-line
stub and the long branch stub is 8-byte aligned. It modifies the epilogue and
fentry stub building functions to accept the final image pointer and emit a
conditional NOP if needed.

> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 7b07b43575f11..f2e0f9755e653 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
[ ... ]
> @@ -49,11 +49,39 @@ asm (
>  "	.popsection				;"
>  );
>  
> -void bpf_jit_build_fentry_stubs(u32 *image, struct codegen_context *ctx)
> +void bpf_jit_build_fentry_stubs(u32 *image, u32 *fimage, struct codegen_context *ctx)
>  {
>  	int ool_stub_idx, long_branch_stub_idx;
> +	int ool_stub_sz;
>  
>  	/*
> +	 * In the final pass, align the mis-aligned dummy_tramp_addr field
> +	 * in the fimage. The alignment NOP must appear before OOL stub,
> +	 * to make ool_stub_idx & long_branch_stub_idx constant from end.
> +	 *
> +	 * dummy_tramp_addr must be 8-byte aligned for load-register
> +	 * compatibility. The fimage can be non 8-byte aligned, so final
> +	 * alignment depends on start of fimage and the stub's instruction
> +	 * count offset. The OOL stub size is 4 instructions (with
> +	 * CONFIG_PPC_FTRACE_OUT_OF_LINE) or 3 instructions (without)
> +	 * before dummy_tramp_addr.
> +	 *
> +	 * Emit a NOP here if (ctx->idx + ool_stub_sz) is odd, so that
> +	 * dummy_tramp_addr lands at an even instruction offset (== 8-byte
> +	 * aligned from an 8-byte aligned base).
> +	 *
> +	 * In pass=0 when image==NULL, conservatively account for space
> +	 * required to accommodate alignment NOP. In case final pass skips
> +	 * emitting alignment NOP, the image buffer have 4 spare bytes and
> +	 * jited_len signifies correct program size.
> +	 */
> +
> +	ool_stub_sz = IS_ENABLED(CONFIG_PPC_FTRACE_OUT_OF_LINE) ? 16 : 12;
> +	if (!image || !IS_ALIGNED((unsigned long)fimage + ctx->idx*4 + ool_stub_sz, SZL))
> +		EMIT(PPC_RAW_NOP());
> +

[Severity: High]
Does this conditional NOP break the JIT convergence loop in
bpf_int_jit_compile()?

If the body size shrinks by an odd number of instructions in the final JIT
pass, this could flip the alignment and cause an extra padding NOP to be
emitted.

This would increase the epilogue size by 4 bytes while the total proglen
remains constant. Could this cause the JIT loop to terminate prematurely while
exit_addr still points to the old epilogue start, skipping the first
instruction of the epilogue and potentially leading to register corruption?

>  	/*
> -	 *      nop     // optional, for alignment of dummy_tramp_addr
[ ... ]
> @@ -97,7 +126,8 @@ 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)
> +int bpf_jit_emit_exit_insn(u32 *image, u32 *fimage, struct codegen_context *ctx,
> +							int tmp_reg, long exit_addr)
>  {
>  	if (!exit_addr || is_offset_in_branch_range(exit_addr - (ctx->idx * 4))) {

[Severity: Critical]
This is a pre-existing issue, but does the !exit_addr condition falsely
report large branches as in-range during Pass 0?

In bpf_int_jit_compile(), the true epilogue offset is populated after the
second Pass 0 loop, meaning during all Pass 0 execution, exit_addr is
evaluated as 0. This bypasses the branch range evaluation and falsely
predicts only 4 bytes per BPF_EXIT.

When exit_addr is resolved in Pass 1 for large programs (>32MB) that exceed
the branch range, could this cause an under-calculated allocation size,
leading to an out-of-bounds write when the inline epilogue is emitted?

>  		PPC_JMP(exit_addr);
[ ... ]
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
> index bfdc50740da8e..1cf12edf0343f 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
[ ... ]
> @@ -1150,6 +1150,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>  			 */
>  			if (i != flen - 1) {
>  				ret = bpf_jit_emit_exit_insn(image, ctx, _R0, exit_addr);
> +				ret = bpf_jit_emit_exit_insn(image, fimage,
> +								ctx, _R0, exit_addr);
>  				if (ret)
>  					return ret;
>  			}

[Severity: Low]
Is the old 4-argument call to bpf_jit_emit_exit_insn() accidentally left here?

Leaving this intact right above the newly added 5-argument call appears to
directly break the build on 32-bit PowerPC architectures due to too few
arguments.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786099877.git.skb99@linux.ibm.com?part=1