x86: Fix early boot crash on gcc-10, third try

commit a9a3ed1eff3601b63aea4fb462d8b3b92c7c1e7e upstream.

... or the odyssey of trying to disable the stack protector for the
function which generates the stack canary value.

The whole story started with Sergei reporting a boot crash with a kernel
built with gcc-10:

  Kernel panic — not syncing: stack-protector: Kernel stack is corrupted in: start_secondary
  CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.6.0-rc5—00235—gfffb08b37df9 #139
  Hardware name: Gigabyte Technology Co., Ltd. To be filled by O.E.M./H77M—D3H, BIOS F12 11/14/2013
  Call Trace:
    dump_stack
    panic
    ? start_secondary
    __stack_chk_fail
    start_secondary
    secondary_startup_64
  -—-[ end Kernel panic — not syncing: stack—protector: Kernel stack is corrupted in: start_secondary

This happens because gcc-10 tail-call optimizes the last function call
in start_secondary() - cpu_startup_entry() - and thus emits a stack
canary check which fails because the canary value changes after the
boot_init_stack_canary() call.

To fix that, the initial attempt was to mark the one function which
generates the stack canary with:

  __attribute__((optimize("-fno-stack-protector"))) ... start_secondary(void *unused)

however, using the optimize attribute doesn't work cumulatively
as the attribute does not add to but rather replaces previously
supplied optimization options - roughly all -fxxx options.

The key one among them being -fno-omit-frame-pointer and thus leading to
not present frame pointer - frame pointer which the kernel needs.

The next attempt to prevent compilers from tail-call optimizing
the last function call cpu_startup_entry(), shy of carving out
start_secondary() into a separate compilation unit and building it with
-fno-stack-protector, was to add an empty asm("").

This current solution was short and sweet, and reportedly, is supported
by both compilers but we didn't get very far this time: future (LTO?)
optimization passes could potentially eliminate this, which leads us
to the third attempt: having an actual memory barrier there which the
compiler cannot ignore or move around etc.

That should hold for a long time, but hey we said that about the other
two solutions too so...

Reported-by: Sergei Trofimovich <slyfox@gentoo.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Tested-by: Kalle Valo <kvalo@codeaurora.org>
Cc: <stable@vger.kernel.org>
Link: https://lkml.kernel.org/r/20200314164451.346497-1-slyfox@gentoo.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
index 8ec97a62c245175e87d96c0376eb962f51f8e91d..9c556ea2eaa728c959d159d6ed7671a47732e6df 100644 (file)
--- a/arch/x86/include/asm/stackprotector.h
+++ b/arch/x86/include/asm/stackprotector.h
@@ -55,8 +55,13 @@
 /*
  * Initialize the stackprotector canary value.
  *
- * NOTE: this must only be called from functions that never return,
+ * NOTE: this must only be called from functions that never return
  * and it must always be inlined.
+ *
+ * In addition, it should be called from a compilation unit for which
+ * stack protector is disabled. Alternatively, the caller should not end
+ * with a function call which gets tail-call optimized as that would
+ * lead to checking a modified canary value.
  */
 static __always_inline void boot_init_stack_canary(void)
 {

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6489067b78a4eafa590aaedb31105d4459860365..8783d065f927634e1e3a9d6ca34194716d9cedcc 100644 (file)
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -269,6 +269,14 @@ static void notrace start_secondary(void *unused)
 
        wmb();
        cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
+
+       /*
+        * Prevent tail call to cpu_startup_entry() because the stack protector
+        * guard has been changed a couple of function calls up, in
+        * boot_init_stack_canary() and must not be checked before tail calling
+        * another function.
+        */
+       prevent_tail_call_optimization();
 }
 
 /**

diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index e3b18ad49889afc5ae35d2e2796aecd108a93819..41fd4c123165aaa6ad317f2b07831526188ce962 100644 (file)
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -89,6 +89,7 @@ asmlinkage __visible void cpu_bringup_and_idle(void)
 {
        cpu_bringup();
        cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
+       prevent_tail_call_optimization();
 }
 
 void xen_smp_intr_free_pv(unsigned int cpu)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 75112aa8064e857d4bd0b7da37a6f821e2049d77..fbb6490c1e0947b1c9c68812c0e8c0aee2e503ef 100644 (file)
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -351,4 +351,10 @@ static inline void *offset_to_ptr(const int *off)
        compiletime_assert(__native_word(t),                            \
                "Need native word sized stores/loads for atomicity.")
 
+/*
+ * This is needed in functions which generate the stack canary, see
+ * arch/x86/kernel/smpboot.c::start_secondary() for an example.
+ */
+#define prevent_tail_call_optimization()       mb()
+
 #endif /* __LINUX_COMPILER_H */

diff --git a/init/main.c b/init/main.c
index 38a603f62b7bb84529af26894929342ad4d0bf41..ec78f2312610e0530d78de3346308bcee9338f11 100644 (file)
--- a/init/main.c
+++ b/init/main.c
@@ -735,6 +735,8 @@ asmlinkage __visible void __init start_kernel(void)
 
        /* Do the rest non-__init'ed, we're now alive */
        rest_init();
+
+       prevent_tail_call_optimization();
 }
 
 /* Call all constructor functions linked into the kernel. */