linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Borislav Petkov <bp@alien8.de>
To: Arnd Bergmann <arnd@arndb.de>
Cc: "Arvind Sankar" <nivedita@alum.mit.edu>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	"Andy Lutomirski" <luto@amacapital.net>,
	"Nick Desaulniers" <ndesaulniers@google.com>,
	"Kalle Valo" <kvalo@codeaurora.org>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	"Kees Cook" <keescook@chromium.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Michael Matz" <matz@suse.de>, "Martin Liška" <mliska@suse.cz>,
	"Jakub Jelinek" <jakub@redhat.com>
Subject: [PATCH] x86: Fix early boot crash on gcc-10, third try
Date: Thu, 14 May 2020 15:27:06 +0200	[thread overview]
Message-ID: <20200514132706.GB9266@zn.tnic> (raw)
In-Reply-To: <CAK8P3a1Cfzu7L30bFP-Sf2_GbkN_10CCJsbefTXyfnGF16uHMA@mail.gmail.com>

Ok,

here's the new version. Changes are:

- It does mb() now (Linus).
- I've added a call to prevent_tail_call_optimization() in init/main.c
  because it does generate the stack canary there too. This is a
  future-proof thing. (Arvind).
- Dropped Reviewed-by tags.
- Dropped compiler checking from the branch (Linus).
- Added Cc:stable because gcc10 has released already, apparently.

Testing with gcc10 passes after making sure that without it it would
cause the tailcall optimization and fail stack check.

Plan is to send it to Linus on the weekend so that it makes it into 5.7.

Thx.

---
From: Borislav Petkov <bp@suse.de>

... 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>
Cc: <stable@vger.kernel.org>
Link: https://lkml.kernel.org/r/20200314164451.346497-1-slyfox@gentoo.org
---
 arch/x86/include/asm/stackprotector.h | 7 ++++++-
 arch/x86/kernel/smpboot.c             | 8 ++++++++
 arch/x86/xen/smp_pv.c                 | 1 +
 include/linux/compiler.h              | 6 ++++++
 init/main.c                           | 2 ++
 5 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
index 91e29b6a86a5..9804a7957f4e 100644
--- 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 fe3ab9632f3b..4f275ac7830b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -266,6 +266,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 8fb8a50a28b4..f2adb63b2d7c 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -93,6 +93,7 @@ asmlinkage __visible void cpu_bringup_and_idle(void)
 	cpu_bringup();
 	boot_init_stack_canary();
 	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 034b0a644efc..448c91bf543b 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -356,4 +356,10 @@ static inline void *offset_to_ptr(const int *off)
 /* &a[0] degrades to a pointer: a different type from an array */
 #define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
 
+/*
+ * 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 a48617f2e5e5..74ac7e48ce02 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1001,6 +1001,8 @@ asmlinkage __visible void __init start_kernel(void)
 
 	/* Do the rest non-__init'ed, we're now alive */
 	arch_call_rest_init();
+
+	prevent_tail_call_optimization();
 }
 
 /* Call all constructor functions linked into the kernel. */
-- 
2.21.0

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

  reply	other threads:[~2020-05-14 13:27 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-09 12:06 [PATCH net-next 1/2] ath10k: fix gcc-10 zero-length-bounds warnings Arnd Bergmann
2020-05-09 12:06 ` [PATCH net-next 2/2] ath10k: fix ath10k_pci struct layout Arnd Bergmann
2020-05-11 12:05   ` Kalle Valo
2020-05-11 12:17     ` Kalle Valo
2020-05-11 12:39       ` Arnd Bergmann
2020-05-13  6:50       ` gcc-10: kernel stack is corrupted and fails to boot Kalle Valo
2020-05-13  8:49         ` Arnd Bergmann
2020-05-13 12:45           ` Kalle Valo
2020-05-13 13:45             ` Arnd Bergmann
2020-05-13 15:31               ` Kalle Valo
2020-05-13 16:00                 ` Arnd Bergmann
2020-05-13 16:07                   ` David Laight
2020-05-14  9:13                 ` Harald Arnesen
2020-05-13 15:48         ` Arvind Sankar
2020-05-13 21:28           ` Arnd Bergmann
2020-05-13 21:41             ` Borislav Petkov
2020-05-13 21:49               ` Arnd Bergmann
2020-05-13 22:20                 ` Borislav Petkov
2020-05-13 22:51                   ` Arvind Sankar
2020-05-13 23:13                   ` Linus Torvalds
2020-05-13 23:36                     ` Borislav Petkov
2020-05-14  0:11                       ` Linus Torvalds
2020-05-14  0:51                         ` Nick Desaulniers
2020-05-14  2:20                           ` Linus Torvalds
2020-05-14  3:50                             ` Andy Lutomirski
     [not found]                               ` <CAHk-=wgiGxRgJGS-zyer1C_x2MQUVo6iZn0=aJyuFTqJWk-mpA@mail.gmail.com>
2020-05-14  5:22                                 ` Arvind Sankar
2020-05-14  8:40                                   ` Arnd Bergmann
2020-05-14 13:27                                     ` Borislav Petkov [this message]
2020-05-14 14:45                                       ` [PATCH] x86: Fix early boot crash on gcc-10, third try Kalle Valo
2020-05-14 15:50                                     ` gcc-10: kernel stack is corrupted and fails to boot Arvind Sankar
2020-05-14  8:11                             ` David Laight
2020-05-13 23:07                 ` Linus Torvalds
2020-05-09 15:48 ` [PATCH net-next 1/2] ath10k: fix gcc-10 zero-length-bounds warnings Gustavo A. R. Silva
2020-05-11 12:02   ` Kalle Valo
2020-05-11 12:46     ` Arnd Bergmann
2020-05-11 13:09       ` Kalle Valo
2020-05-11 13:47         ` Arnd Bergmann
2020-05-12  7:33 ` Kalle Valo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200514132706.GB9266@zn.tnic \
    --to=bp@alien8.de \
    --cc=arnd@arndb.de \
    --cc=jakub@redhat.com \
    --cc=keescook@chromium.org \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=matz@suse.de \
    --cc=mliska@suse.cz \
    --cc=ndesaulniers@google.com \
    --cc=nivedita@alum.mit.edu \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).