Re: [PATCH v4 1/5] 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:
- [Critical] JIT Buffer Overflow due to undersized pass 0 allocation.
- [High] Flawed alignment arithmetic actively misaligns the trampoline address.
- [Medium] Inline epilogues silently bypass the alignment logic.
--

commit ce3a7a39e286dcf5c4eaad0616ec652506a106fa
Author: Abhishek Dubey <adubey@linux.ibm.com>
Subject: powerpc/bpf: fix alignment of long branch trampoline address

This commit attempts to align the dummy trampoline address field between
the OOL stub and the long branch stub to 8 bytes for memory compatibility.
It does this by emitting an optional NOP depending on the alignment of the
instruction pointer.

> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -49,11 +49,34 @@ 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;
>  
>  	/*
> +	 * 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.
> +	 */
> +#ifdef CONFIG_PPC64
> +	if (fimage && image) {

Since this condition evaluates to false during pass 0 (where fimage and
image are NULL), the alignment NOP is skipped and the required buffer size
in proglen is calculated without it.

During the final passes (1 and 2), if the NOP is emitted, could this increase
the instruction count beyond proglen and overflow into the subsequent
exception table or fixups? A later patch in the series may alter this area,
but does this patch introduce a self-contained memory corruption regression?

> +		/*
> +		 * pc points to first instruction of OOL stub,
> +		 * dummy_tramp_addr is past 4/3 instructions depending on
> +		 * CONFIG_PPC_FTRACE_OUT_OF_LINE is enabled/not respectively.
> +		 *
> +		 * The decision to emit alignment NOP must depend on the alignment
> +		 * of dummy_tramp_addr field.
> +		 */
> +		unsigned long pc = (unsigned long)fimage + CTX_NIA(ctx);
> +		pc += IS_ENABLED(CONFIG_PPC_FTRACE_OUT_OF_LINE) ? 4 : 3;

Because pc is an unsigned long byte address, adding 4 or 3 adds bytes
instead of the intended 16 or 12 bytes (since PowerPC instructions are 4
bytes long).

Adding 4 bytes to an 8-byte aligned address makes it unaligned. Does this
flawed arithmetic force the code to emit a NOP when the target address is
already aligned, thereby misaligning it?

> +		if (!IS_ALIGNED(pc, 8))
> +			EMIT(PPC_RAW_NOP());
> +	}
> +#endif

[ ... ]

> @@ -107,7 +131,7 @@ int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg,
>  		PPC_JMP(ctx->alt_exit_addr);
>  	} else {
>  		ctx->alt_exit_addr = ctx->idx * 4;
> -		bpf_jit_build_epilogue(image, ctx);
> +		bpf_jit_build_epilogue(image, NULL, ctx);
>  	}

Because fimage is strictly passed as NULL here, the if (fimage && image)
check in bpf_jit_build_fentry_stubs() will always evaluate to false.

Will this cause the alignment NOP to be unconditionally skipped for all inline
epilogues, leaving their dummy_tramp_addr fields unaligned?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517214043.12975-1-adubey@linux.ibm.com?part=1