linux-kernel - Re: [PATCH RFC] powerpc: Panic on jump label code patching failure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c841e220-9ab4-400f-bb9a-18c6981488d9@csgroup.eu>
Date: Fri, 5 Sep 2025 08:34:49 +0200
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Andrew Donnellan <ajd@...ux.ibm.com>, linuxppc-dev@...ts.ozlabs.org,
 linux-kernel@...r.kernel.org, maddy@...ux.ibm.com, mpe@...erman.id.au,
 peterz@...radead.org, jpoimboe@...nel.org, jbaron@...mai.com
Cc: npiggin@...il.com, rostedt@...dmis.org, ardb@...nel.org,
 Erhard Furtner <erhard_f@...lbox.org>
Subject: Re: [PATCH RFC] powerpc: Panic on jump label code patching failure



Le 05/09/2025 à 08:11, Andrew Donnellan a écrit :
> If patch_branch() or patch_instruction() fails while updating a jump
> label, we presently fail silently, leading to unpredictable behaviour
> later on.
> 
> Change arch_jump_label_transform() to panic on a code patching failure,
> matching the existing behaviour of arch_static_call_transform().
> 
> Reported-by: Erhard Furtner <erhard_f@...lbox.org>

checkpatch.pl is not happy:

WARNING: Use lore.kernel.org archive links when possible - see 
https://lore.kernel.org/lists.html
#131:
   <https://lists.ozlabs.org/pipermail/linuxppc-dev/>

WARNING: Reported-by: should be immediately followed by Closes: with a 
URL to the report
#173:
Reported-by: Erhard Furtner <erhard_f@...lbox.org>
Signed-off-by: Andrew Donnellan <ajd@...ux.ibm.com>

total: 0 errors, 2 warnings, 0 checks, 16 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or 
--fix-inplace.

/home/chleroy/Téléchargements/RFC-powerpc-Panic-on-jump-label-code-patching-failure.patch 
has style problems, please review.

NOTE: If any of the errors are false positives, please report
       them to the maintainer, see CHECKPATCH in MAINTAINERS.

> Signed-off-by: Andrew Donnellan <ajd@...ux.ibm.com>

Reviewed-by: Christophe Leroy <christophe.leroy@...roup.eu>

> 
> ---
> 
> Ran into this while debugging an issue that Erhard reported to me about my
> PAGE_TABLE_CHECK series on a G4, where updating a static key failed
> silently, but only for one call site, leading to an incorrect reference
> count later on. This looks to be due to the issue fixed in [0]. A loud
> failure would have saved us all considerable debugging time.
> 
> Should I change the return type of arch_jump_label_transform() and handle
> this in an arch-independent way? Are there other users of code patching
> in powerpc that ought to be hardened?whon

I think all callers of patch_branch() and patch_instruction() should 
check returned value. Several already do. I think we should fix the ones 
which don't then make patch_branch() and patch_instruction() __must_check

> 
> Or is this excessive?
> 
> [0] https://patchwork.ozlabs.org/project/linuxppc-dev/patch/4b5e6eb281d7b1ea77619bee17095f905a125168.1757003584.git.christophe.leroy@csgroup.eu/
> ---
>   arch/powerpc/kernel/jump_label.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/jump_label.c b/arch/powerpc/kernel/jump_label.c
> index 2659e1ac8604..80d41ed7ac50 100644
> --- a/arch/powerpc/kernel/jump_label.c
> +++ b/arch/powerpc/kernel/jump_label.c
> @@ -12,9 +12,14 @@ void arch_jump_label_transform(struct jump_entry *entry,
>   			       enum jump_label_type type)
>   {
>   	u32 *addr = (u32 *)jump_entry_code(entry);
> +	int err;
>   
>   	if (type == JUMP_LABEL_JMP)
> -		patch_branch(addr, jump_entry_target(entry), 0);
> +		err = patch_branch(addr, jump_entry_target(entry), 0);
>   	else
> -		patch_instruction(addr, ppc_inst(PPC_RAW_NOP()));
> +		err = patch_instruction(addr, ppc_inst(PPC_RAW_NOP()));
> +
> +	if (err)
> +		panic("%s: patching failed, err %d, type %d, addr %pS, target %pS\n",
> +		      __func__, err, type, addr, (void *)jump_entry_target(entry));
>   }