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]
Date: Wed, 3 Apr 2024 19:10:17 +0200
From: Klara Modin <klarasmodin@...il.com>
To: Borislav Petkov <bp@...en8.de>, Josh Poimboeuf <jpoimboe@...nel.org>
Cc: "Kaplan, David" <David.Kaplan@....com>, Ingo Molnar <mingo@...nel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-tip-commits@...r.kernel.org" <linux-tip-commits@...r.kernel.org>,
 "Peter Zijlstra (Intel)" <peterz@...radead.org>,
 "x86@...nel.org" <x86@...nel.org>, David Howells <dhowells@...hat.com>
Subject: Re: [PATCH -v2] x86/retpoline: Ensure default return thunk isn't used
 at runtime

Hi,

On 2024-02-12 11:43, Borislav Petkov wrote:
> On Wed, Feb 07, 2024 at 11:49:19AM -0800, Josh Poimboeuf wrote:
>> LGTM, thanks!
> 
> Thanks, had to hoist up both THUNK macros into the header to make that
> nuisance 32-bit build too :)
> 
> ---
> 
> commit 4461438a8405e800f90e0e40409e5f3d07eed381 (HEAD -> refs/heads/tip-x86-bugs)
> Author: Josh Poimboeuf <jpoimboe@...nel.org>
> Date:   Wed Jan 3 19:36:26 2024 +0100
> 
>      x86/retpoline: Ensure default return thunk isn't used at runtime
>      
>      Make sure the default return thunk is not used after all return
>      instructions have been patched by the alternatives because the default
>      return thunk is insufficient when it comes to mitigating Retbleed or
>      SRSO.
>      
>      Fix based on an earlier version by David Kaplan <david.kaplan@....com>.
>      
>        [ bp: Fix the compilation error of warn_thunk_thunk being an invisible
>              symbol, hoist thunk macro into calling.h ]
>      
>      Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.org>
>      Co-developed-by: Borislav Petkov (AMD) <bp@...en8.de>
>      Signed-off-by: Borislav Petkov (AMD) <bp@...en8.de>
>      Link: https://lore.kernel.org/r/20231010171020.462211-4-david.kaplan@amd.com
>      Link: https://lore.kernel.org/r/20240104132446.GEZZaxnrIgIyat0pqf@fat_crate.local
> 

With this patch/commit, one of my machines (older P4 Xeon, 32-bit only) 
hangs on boot with CONFIG_RETHUNK=y / CONFIG_MITIGATION_RETHUNK=y.

> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> index 39e069b68c6e..bd31b2534053 100644
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -426,3 +426,63 @@ For 32-bit we have the following conventions - kernel is built with
>   .endm
>   
>   #endif /* CONFIG_SMP */
> +
> +#ifdef CONFIG_X86_64
> +
> +/* rdi:	arg1 ... normal C conventions. rax is saved/restored. */
> +.macro THUNK name, func
> +SYM_FUNC_START(\name)
> +	pushq %rbp
> +	movq %rsp, %rbp
> +
> +	pushq %rdi
> +	pushq %rsi
> +	pushq %rdx
> +	pushq %rcx
> +	pushq %rax
> +	pushq %r8
> +	pushq %r9
> +	pushq %r10
> +	pushq %r11
> +
> +	call \func
> +
> +	popq %r11
> +	popq %r10
> +	popq %r9
> +	popq %r8
> +	popq %rax
> +	popq %rcx
> +	popq %rdx
> +	popq %rsi
> +	popq %rdi
> +	popq %rbp
> +	RET
> +SYM_FUNC_END(\name)
> +	_ASM_NOKPROBE(\name)
> +.endm
> +
> +#else /* CONFIG_X86_32 */
> +
> +/* put return address in eax (arg1) */
> +.macro THUNK name, func, put_ret_addr_in_eax=0
> +SYM_CODE_START_NOALIGN(\name)
> +	pushl %eax
> +	pushl %ecx
> +	pushl %edx
> +
> +	.if \put_ret_addr_in_eax
> +	/* Place EIP in the arg1 */
> +	movl 3*4(%esp), %eax
> +	.endif
> +
> +	call \func
> +	popl %edx
> +	popl %ecx
> +	popl %eax
> +	RET
> +	_ASM_NOKPROBE(\name)
> +SYM_CODE_END(\name)
> +	.endm
> +
> +#endif
> diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S
> index 8c8d38f0cb1d..582731f74dc8 100644
> --- a/arch/x86/entry/entry.S
> +++ b/arch/x86/entry/entry.S
> @@ -7,6 +7,8 @@
>   #include <linux/linkage.h>
>   #include <asm/msr-index.h>
>   
> +#include "calling.h"
> +
>   .pushsection .noinstr.text, "ax"
>   
>   SYM_FUNC_START(entry_ibpb)
> @@ -20,3 +22,5 @@ SYM_FUNC_END(entry_ibpb)
>   EXPORT_SYMBOL_GPL(entry_ibpb);
>   
>   .popsection
> +
> +THUNK warn_thunk_thunk, __warn_thunk
> diff --git a/arch/x86/entry/thunk_32.S b/arch/x86/entry/thunk_32.S
> index 0103e103a657..da37f42f4549 100644
> --- a/arch/x86/entry/thunk_32.S
> +++ b/arch/x86/entry/thunk_32.S
> @@ -4,33 +4,15 @@
>    * Copyright 2008 by Steven Rostedt, Red Hat, Inc
>    *  (inspired by Andi Kleen's thunk_64.S)
>    */
> -	#include <linux/export.h>
> -	#include <linux/linkage.h>
> -	#include <asm/asm.h>
>   
> -	/* put return address in eax (arg1) */
> -	.macro THUNK name, func, put_ret_addr_in_eax=0
> -SYM_CODE_START_NOALIGN(\name)
> -	pushl %eax
> -	pushl %ecx
> -	pushl %edx
> +#include <linux/export.h>
> +#include <linux/linkage.h>
> +#include <asm/asm.h>
>   
> -	.if \put_ret_addr_in_eax
> -	/* Place EIP in the arg1 */
> -	movl 3*4(%esp), %eax
> -	.endif
> +#include "calling.h"
>   
> -	call \func
> -	popl %edx
> -	popl %ecx
> -	popl %eax
> -	RET
> -	_ASM_NOKPROBE(\name)
> -SYM_CODE_END(\name)
> -	.endm
> -
> -	THUNK preempt_schedule_thunk, preempt_schedule
> -	THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace
> -	EXPORT_SYMBOL(preempt_schedule_thunk)
> -	EXPORT_SYMBOL(preempt_schedule_notrace_thunk)
> +THUNK preempt_schedule_thunk, preempt_schedule
> +THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace
> +EXPORT_SYMBOL(preempt_schedule_thunk)
> +EXPORT_SYMBOL(preempt_schedule_notrace_thunk)
>   
> diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
> index 416b400f39db..119ebdc3d362 100644
> --- a/arch/x86/entry/thunk_64.S
> +++ b/arch/x86/entry/thunk_64.S
> @@ -9,39 +9,6 @@
>   #include "calling.h"
>   #include <asm/asm.h>
>   
> -	/* rdi:	arg1 ... normal C conventions. rax is saved/restored. */
> -	.macro THUNK name, func
> -SYM_FUNC_START(\name)
> -	pushq %rbp
> -	movq %rsp, %rbp
> -
> -	pushq %rdi
> -	pushq %rsi
> -	pushq %rdx
> -	pushq %rcx
> -	pushq %rax
> -	pushq %r8
> -	pushq %r9
> -	pushq %r10
> -	pushq %r11
> -
> -	call \func
> -
> -	popq %r11
> -	popq %r10
> -	popq %r9
> -	popq %r8
> -	popq %rax
> -	popq %rcx
> -	popq %rdx
> -	popq %rsi
> -	popq %rdi
> -	popq %rbp
> -	RET
> -SYM_FUNC_END(\name)
> -	_ASM_NOKPROBE(\name)
> -	.endm
> -
>   THUNK preempt_schedule_thunk, preempt_schedule
>   THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace
>   EXPORT_SYMBOL(preempt_schedule_thunk)
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index 2c0679ebe914..55754617eaee 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -357,6 +357,8 @@ extern void entry_ibpb(void);
>   
>   extern void (*x86_return_thunk)(void);
>   
> +extern void __warn_thunk(void);
> +
>   #ifdef CONFIG_MITIGATION_CALL_DEPTH_TRACKING
>   extern void call_depth_return_thunk(void);
>   
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index f2775417bda2..a78892b0f823 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -2850,3 +2850,8 @@ ssize_t cpu_show_gds(struct device *dev, struct device_attribute *attr, char *bu
>   	return cpu_show_common(dev, attr, buf, X86_BUG_GDS);
>   }
>   #endif
> +
> +void __warn_thunk(void)
> +{
> +	WARN_ONCE(1, "Unpatched return thunk in use. This should not happen!\n");
> +}


> diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
> index 0045153ba222..721b528da9ac 100644
> --- a/arch/x86/lib/retpoline.S
> +++ b/arch/x86/lib/retpoline.S
> @@ -369,19 +369,16 @@ SYM_FUNC_END(call_depth_return_thunk)
>    * 'JMP __x86_return_thunk' sites are changed to something else by
>    * apply_returns().
>    *
> - * This should be converted eventually to call a warning function which
> - * should scream loudly when the default return thunk is called after
> - * alternatives have been applied.
> - *
> - * That warning function cannot BUG() because the bug splat cannot be
> - * displayed in all possible configurations, leading to users not really
> - * knowing why the machine froze.
> + * The ALTERNATIVE below adds a really loud warning to catch the case
> + * where the insufficient default return thunk ends up getting used for
> + * whatever reason like miscompilation or failure of
> + * objtool/alternatives/etc to patch all the return sites.
>    */
>   SYM_CODE_START(__x86_return_thunk)
>   	UNWIND_HINT_FUNC
>   	ANNOTATE_NOENDBR
> -	ANNOTATE_UNRET_SAFE
> -	ret
> +	ALTERNATIVE __stringify(ANNOTATE_UNRET_SAFE; ret), \
> +		   "jmp warn_thunk_thunk", X86_FEATURE_ALWAYS
>   	int3
>   SYM_CODE_END(__x86_return_thunk)
>   EXPORT_SYMBOL(__x86_return_thunk)
> 

This seems to be the problematic snippet. Reverting it alone fixes the 
issue for me. I wonder if it could have anything to do with the previous 
comment text?

Please let me know if there's anything else you need.

Kind regards,
Klara Modin
Download attachment "xeon2p-console.log.gz" of type "application/gzip" (3145 bytes)

View attachment "xeon2p-no-boot-bisect.log" of type "text/x-log" (2654 bytes)

Download attachment "xeon2p-working.log.gz" of type "application/gzip" (12934 bytes)

Download attachment "config.gz" of type "application/gzip" (32562 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ