Re: [PATCH v4 2/5] powerpc/bpf: Move out dummy_tramp_addr after Long branch stub

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The JIT alignment logic for `dummy_tramp_addr` is skipped during the sizing pass but executed during code generation, causing buffer overflows and BPF trampoline corruption.
- [High] The alignment prediction logic for `dummy_tramp_addr` incorrectly adds unscaled bytes instead of instructions, breaking the alignment and causing `bpf_arch_text_poke()` to fail.
- [Medium] Commit message claims an implementation that does not exist in the code.
--

commit e4939a5c93f678ddd053f906d998b2151b87ea0f
Author: Abhishek Dubey <adubey@linux.ibm.com>

powerpc/bpf: Move out dummy_tramp_addr after Long branch stub

This commit moves the dummy_tramp_addr field to the bottom of the long
branch stub to allow uninterrupted disassembly. It also updates the offset
calculation for bpf_arch_text_poke() to reflect the new location.

> Move the long branch address space to the bottom of the long
> branch stub. This allows uninterrupted disassembly until the
> last 8 bytes. Exclude these last bytes from the overall
> program length to prevent failure in assembly generation.

The commit message states that the last bytes are excluded from the overall
program length. Is this description accurate?

Looking at the code, ctx->idx is not decremented, and the length calculation
still appears to include the 8 bytes of dummy_tramp_addr.

> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index ef7614177cb1f..b73bc9295c31d 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -57,19 +57,21 @@ void bpf_jit_build_fentry_stubs(u32 *image, u32 *fimage, struct codegen_context
[ ... ]
>  #ifdef CONFIG_PPC64
>  	if (fimage && image) {
>  		/*
>  		 * pc points to first instruction of OOL stub,
> -		 * dummy_tramp_addr is past 4/3 instructions depending on
> +		 * dummy_tramp_addr is past 11/10 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;
> +		pc += IS_ENABLED(CONFIG_PPC_FTRACE_OUT_OF_LINE) ? 11 : 10;
>  
>  		if (!IS_ALIGNED(pc, 8))
>  			EMIT(PPC_RAW_NOP());
>  	}
>  #endif

Does this alignment prediction logic incorrectly add unscaled bytes instead
of instructions?

Since pc is a byte address, adding 11 or 10 bytes instead of multiplying by
4 (the instruction size) guarantees pc is no longer a multiple of 8. This
would cause the NOP to be unconditionally emitted, merely shifting the layout
by 4 bytes instead of dynamically padding it.

If the NOP is unconditionally emitted here, could dummy_tramp_addr remain
8-byte misaligned for many BPF programs, causing patch_ulong() to strictly
reject the address and BPF trampoline attachments to fail with -EINVAL?

Additionally, does this conditional alignment logic cause buffer overflows?

During the sizing pass, fimage is NULL, so the NOP emission is skipped and
the program length is calculated without it. During the code generation
pass, the NOP is emitted, causing the generated code to exceed the allocated
length by 4 bytes. Could this silently overwrite JIT metadata allocated
immediately after the program?

[ ... ]
> @@ -1400,10 +1404,12 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type old_t,
>  	 * 1. Update the address in the long branch stub:
>  	 * If new_addr is out of range, we will have to use the long branch stub, so patch new_addr
>  	 * here. Otherwise, revert to dummy_tramp, but only if we had patched old_addr here.
> +	 *
> +	 * dummy_tramp_addr moved to bottom of long branch stub.
>  	 */
>  	if ((new_addr && !is_offset_in_branch_range(new_addr - ip)) ||
>  	    (old_addr && !is_offset_in_branch_range(old_addr - ip)))
> -		ret = patch_ulong((void *)(bpf_func_end - bpf_jit_long_branch_stub - SZL),
> +		ret = patch_ulong((void *)(bpf_func_end - SZL), /* SZL: dummy_tramp_addr offset */
>  				  (new_addr && !is_offset_in_branch_range(new_addr - ip)) ?
>  				  (unsigned long)new_addr : (unsigned long)dummy_tramp);
>  	if (ret)

If the extra NOP is emitted during generation and the layout shifts by 4
bytes, dummy_tramp_addr would be physically located at bpf_func_end - 4.

Since bpf_arch_text_poke() patches bpf_func_end - SZL (which evaluates to
8 bytes), could patch_ulong() overwrite the bctr instruction with the new
target address instead of dummy_tramp_addr, leading to an illegal instruction
panic when the trampoline executes?

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