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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Passing `NULL` for `fimage` in `bpf_jit_emit_exit_insn()` causes incorrect alignment calculations for alternate epilogues, leading to an unaligned 8-byte memory store during JIT compilation and misaligned runtime addresses.
--

--- Patch [1]: [PATCH v10 1/8] powerpc/bpf: fix alignment of long branch trampoline address ---
commit 4bc0d2265cceaf063f46ec43f44292e01148338a
Author: Abhishek Dubey <[email protected]>
powerpc/bpf: fix alignment of long branch trampoline address

This commit attempts to fix the alignment of the dummy trampoline address
field between the out-of-line stub and the long branch stub. It introduces a
NOP padding instruction to ensure the address is 8-byte aligned for
load-register compatibility. It also passes fimage through the epilogue and
fentry stub building functions to calculate the correct alignment.

> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 7b07b43575f11..42046f73c2792 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)
>  {

[ ... ]

> +	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());
> +

[ ... ]

> @@ -81,6 +109,7 @@ void bpf_jit_build_fentry_stubs(u32 *image, struct codegen_context *ctx)
>  	 */
>  	if (image)
>  		*((unsigned long *)&image[ctx->idx]) = (unsigned long)dummy_tramp;
> +
>  	ctx->idx += SZL / 4;
>  	long_branch_stub_idx = ctx->idx;
>  	EMIT(PPC_RAW_MFLR(_R11));

[ ... ]

> @@ -107,7 +136,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);
>  	}
>  
>  	return 0;

[Severity: High]
Does passing NULL for fimage here cause incorrect alignment calculations for
alternate epilogues?

Since bpf_jit_emit_exit_insn() passes NULL to bpf_jit_build_epilogue(), the
fimage argument eventually passed to bpf_jit_build_fentry_stubs() will be NULL.
This means the IS_ALIGNED() check in bpf_jit_build_fentry_stubs() will evaluate
alignment relative to 0 instead of the actual fcode_base.

Because the executable memory allocated by bpf_jit_binary_pack_alloc() only
guarantees 4-byte alignment, the real fimage base might not be 8-byte aligned.
If the NOP calculation evaluates 0 instead of a 4-byte aligned base, it can
incorrectly omit the padding, leading to an unaligned 64-bit store when writing
dummy_tramp:

	if (image)
		*((unsigned long *)&image[ctx->idx]) = (unsigned long)dummy_tramp;

Performing a 64-bit store to a misaligned pointer is undefined behavior in C
and can trigger alignment exceptions. Will this also cause the ld instruction
to load from a misaligned address at runtime, defeating the patch's goal of
load-register compatibility?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1