Re: [PATCH -v2] x86/retpoline: Ensure default return thunk isn't used at runtime

[Klara Modin <klarasmodin@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8053 bytes --]

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@kernel.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@amd.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@kernel.org>
>      Co-developed-by: Borislav Petkov (AMD) <bp@alien8.de>
>      Signed-off-by: Borislav Petkov (AMD) <bp@alien8.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

[-- Attachment #2: xeon2p-console.log.gz --]
[-- Type: application/gzip, Size: 3145 bytes --]

[-- Attachment #3: xeon2p-no-boot-bisect.log --]
[-- Type: text/x-log, Size: 2654 bytes --]

# bad: [39cd87c4eb2b893354f3b850f916353f2658ae6f] Linux 6.9-rc2
# good: [e8f897f4afef0031fe618a8e94127a0934896aba] Linux 6.8
git bisect start 'v6.9-rc2' 'v6.8'
# bad: [e5e038b7ae9da96b93974bf072ca1876899a01a3] Merge tag 'fs_for_v6.9-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs
git bisect bad e5e038b7ae9da96b93974bf072ca1876899a01a3
# bad: [1f440397665f4241346e4cc6d93f8b73880815d1] Merge tag 'docs-6.9' of git://git.lwn.net/linux
git bisect bad 1f440397665f4241346e4cc6d93f8b73880815d1
# bad: [508f34f2381eb84b2335abb970b940aefef50a19] Merge tag 'm68k-for-v6.9-tag1' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k
git bisect bad 508f34f2381eb84b2335abb970b940aefef50a19
# good: [8ede842f669b6f78812349bbef4d1efd0fbdafce] Merge tag 'rust-6.9' of https://github.com/Rust-for-Linux/linux
git bisect good 8ede842f669b6f78812349bbef4d1efd0fbdafce
# good: [bfdb395a7cde12d83a623949ed029b0ab38d765b] Merge tag 'x86_mtrr_for_v6.9_rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect good bfdb395a7cde12d83a623949ed029b0ab38d765b
# bad: [685d98211273f60e38a6d361b62d7016c545297e] Merge tag 'x86-core-2024-03-11' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad 685d98211273f60e38a6d361b62d7016c545297e
# good: [b0402403e54ae9eb94ce1cbb53c7def776e97426] Merge tag 'edac_updates_for_v6.9' of git://git.kernel.org/pub/scm/linux/kernel/git/ras/ras
git bisect good b0402403e54ae9eb94ce1cbb53c7def776e97426
# bad: [cb81deefb59de01325ab822f900c13941bfaf67f] x86/idle: Sanitize X86_BUG_AMD_E400 handling
git bisect bad cb81deefb59de01325ab822f900c13941bfaf67f
# good: [3a1d3829e193c091475ceab481c5f8deab385023] x86/percpu: Avoid sparse warning with cast to named address space
git bisect good 3a1d3829e193c091475ceab481c5f8deab385023
# bad: [4461438a8405e800f90e0e40409e5f3d07eed381] x86/retpoline: Ensure default return thunk isn't used at runtime
git bisect bad 4461438a8405e800f90e0e40409e5f3d07eed381
# good: [aefb2f2e619b6c334bcb31de830aa00ba0b11129] x86/bugs: Rename CONFIG_RETPOLINE            => CONFIG_MITIGATION_RETPOLINE
git bisect good aefb2f2e619b6c334bcb31de830aa00ba0b11129
# good: [1da8d2172ce5175118929363fe568e41f24ad3d6] x86/bugs: Rename CONFIG_CPU_IBRS_ENTRY       => CONFIG_MITIGATION_IBRS_ENTRY
git bisect good 1da8d2172ce5175118929363fe568e41f24ad3d6
# good: [0911b8c52c4d68c57d02f172daa55a42bce703f0] x86/bugs: Rename CONFIG_RETHUNK              => CONFIG_MITIGATION_RETHUNK
git bisect good 0911b8c52c4d68c57d02f172daa55a42bce703f0
# first bad commit: [4461438a8405e800f90e0e40409e5f3d07eed381] x86/retpoline: Ensure default return thunk isn't used at runtime

[-- Attachment #4: xeon2p-working.log.gz --]
[-- Type: application/gzip, Size: 12934 bytes --]

[-- Attachment #5: config.gz --]
[-- Type: application/gzip, Size: 32562 bytes --]