[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-kernel
Subject: Re: [PATCH -v2] x86/retpoline: Ensure default return thunk isn't used at runtime
From: Klara Modin <klarasmodin () gmail ! com>
Date: 2024-04-03 17:10:17
Message-ID: 78e0d19c-b77a-4169-a80f-2eef91f4a1d6 () gmail ! com
[Download RAW message or body]
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
["xeon2p-console.log.gz" (application/gzip)]
["xeon2p-no-boot-bisect.log" (text/x-log)]
# 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
["xeon2p-working.log.gz" (application/gzip)]
["config.gz" (application/gzip)]
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic