| 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
| ||
|
Message-ID: <78e0d19c-b77a-4169-a80f-2eef91f4a1d6@gmail.com>
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