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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Preexisting boundary asymmetry in branch range validation causes silent failure and NOP patching when trampoline targets are exactly 32MB away.
--

commit 5b58234b51abd3de6209e164ca3ba6723fb0f569
Author: Abhishek Dubey <adubey@linux.ibm.com>
powerpc/bpf: Move out dummy_tramp_addr after Long branch stub

This patch moves the long branch address field to the bottom of the long
branch stub to prevent disassembly failure. It updates the dummy_tramp_addr
field offset calculation from the end of the program to reflect its new
location.

> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 8ca36a933c7ae..c2717f0d9cdd4 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
[ ... ]
> @@ -1425,10 +1427,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)

[Severity: High]
This is a pre-existing issue, but looking at bpf_arch_text_poke(), there
appears to be a boundary asymmetry in the branch range validation just above
this hunk.

Earlier in the function, the offset is calculated as ip - old_addr and
ip - new_addr:

	if (old_addr) {
		if (is_offset_in_branch_range(ip - old_addr))
			create_branch(&old_inst, ip, (unsigned long)old_addr, branch_flags);
		else
...
	if (new_addr) {
		if (is_offset_in_branch_range(ip - new_addr))
			create_branch(&new_inst, ip, (unsigned long)new_addr, branch_flags);
		else

While in the hunk above it correctly calculates new_addr - ip and
old_addr - ip.

Does this cause silent failures when trampoline targets are exactly 32MB away?

If new_addr is exactly 0x2000000 bytes ahead of ip, the true offset (0x2000000)
is out of range. However, ip - new_addr evaluates to -0x2000000, which
is_offset_in_branch_range() incorrectly sees as IN range.

If this happens, the code enters the block and calls create_branch(), which
correctly calculates the true offset, fails its internal bounds check, and
returns an error.

Because bpf_arch_text_poke() ignores the return value of create_branch() in
this path, does this leave new_inst as a NOP?

If so, bpf_modify_inst() would silently patch a NOP into the execution path
instead of a branch to the long branch stub, dropping the BPF call entirely.

Similarly, if the target is 0x2000000 bytes behind ip, the true offset
-0x2000000 is valid, but ip - old_addr evaluates to 0x2000000, incorrectly
triggering the else path.

Could this mismatch cause bpf_modify_inst() to fail (as the instruction
currently in memory is a direct branch, not a stub branch), leaving the system
unable to detach the BPF program?

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