diff mbox series

[v2] x86/stackprotector: Work around strict Clang TLS symbol requirements

Message ID 20241009124352.3105119-2-ardb+git@google.com (mailing list archive)
State Mainlined
Commit 577c134d311b9b94598d7a0c86be1f431f823003
Headers show
Series [v2] x86/stackprotector: Work around strict Clang TLS symbol requirements | expand

Commit Message

Ard Biesheuvel Oct. 9, 2024, 12:43 p.m. UTC 
From: Ard Biesheuvel <ardb@kernel.org>

GCC and Clang both implement stack protector support based on Thread
Local Storage (TLS) variables, and this is used in the kernel to
implement per-task stack cookies, by copying a task's stack cookie into
a per-CPU variable every time it is scheduled in.

Both now also implement -mstack-protector-guard-symbol=, which permits
the TLS variable to be specified directly. This is useful because it
will allow us to move away from using a fixed offset of 40 bytes into
the per-CPU area on x86_64, which requires a lot of special handling in
the per-CPU code and the runtime relocation code.

However, while GCC is rather lax in its implementation of this command
line option, Clang actually requires that the provided symbol name
refers to a TLS variable (i.e., one declared with __thread), although it
also permits the variable to be undeclared entirely, in which case it
will use an implicit declaration of the right type.

The upshot of this is that Clang will emit the correct references to the
stack cookie variable in most cases, e.g.,

   10d:       64 a1 00 00 00 00       mov    %fs:0x0,%eax
                      10f: R_386_32   __stack_chk_guard

However, if a non-TLS definition of the symbol in question is visible in
the same compilation unit (which amounts to the whole of vmlinux if LTO
is enabled), it will drop the per-CPU prefix and emit a load from a
bogus address.

Work around this by using a symbol name that never occurs in C code, and
emit it as an alias in the linker script.

Fixes: 3fb0fdb3bbe7 ("x86/stackprotector/32: Make the canary into a regular percpu variable")
Cc: <stable@vger.kernel.org>
Cc: Fangrui Song <i@maskray.me>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Uros Bizjak <ubizjak@gmail.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Andy Lutomirski <luto@kernel.org>
Link: https://github.com/ClangBuiltLinux/linux/issues/1854
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
v2: add declaration of alias to asm-prototypes.h, but expose it only to
    genksyms

 arch/x86/Makefile                     |  5 +++--
 arch/x86/entry/entry.S                | 16 ++++++++++++++++
 arch/x86/include/asm/asm-prototypes.h |  3 +++
 arch/x86/kernel/cpu/common.c          |  2 ++
 arch/x86/kernel/vmlinux.lds.S         |  3 +++
 5 files changed, 27 insertions(+), 2 deletions(-)

Comments

Kees Cook Oct. 14, 2024, 8:59 p.m. UTC | #1 
On Wed, Oct 09, 2024 at 02:43:53PM +0200, Ard Biesheuvel wrote:
> However, if a non-TLS definition of the symbol in question is visible in
> the same compilation unit (which amounts to the whole of vmlinux if LTO
> is enabled), it will drop the per-CPU prefix and emit a load from a
> bogus address.

I take this to mean that x86 32-bit kernels built with the stack
protector and using Clang LTO will crash very quickly?
Ard Biesheuvel Oct. 15, 2024, 10:56 a.m. UTC | #2 
On Mon, 14 Oct 2024 at 22:59, Kees Cook <kees@kernel.org> wrote:
>
> On Wed, Oct 09, 2024 at 02:43:53PM +0200, Ard Biesheuvel wrote:
> > However, if a non-TLS definition of the symbol in question is visible in
> > the same compilation unit (which amounts to the whole of vmlinux if LTO
> > is enabled), it will drop the per-CPU prefix and emit a load from a
> > bogus address.
>
> I take this to mean that x86 32-bit kernels built with the stack
> protector and using Clang LTO will crash very quickly?
>

Yeah. The linked issue is not quite clear, but it does suggest things
are pretty broken in that case.
Nathan Chancellor Oct. 16, 2024, 2:10 a.m. UTC | #3 
On Tue, Oct 15, 2024 at 12:56:57PM +0200, Ard Biesheuvel wrote:
> On Mon, 14 Oct 2024 at 22:59, Kees Cook <kees@kernel.org> wrote:
> >
> > On Wed, Oct 09, 2024 at 02:43:53PM +0200, Ard Biesheuvel wrote:
> > > However, if a non-TLS definition of the symbol in question is visible in
> > > the same compilation unit (which amounts to the whole of vmlinux if LTO
> > > is enabled), it will drop the per-CPU prefix and emit a load from a
> > > bogus address.
> >
> > I take this to mean that x86 32-bit kernels built with the stack
> > protector and using Clang LTO will crash very quickly?
> >
> 
> Yeah. The linked issue is not quite clear, but it does suggest things
> are pretty broken in that case.

Yeah, i386_defconfig with CONFIG_LTO_CLANG_FULL=y explodes on boot for
me without this change:

  [    0.000000] Linux version 6.12.0-rc3-00044-g2f87d0916ce0 (nathan@thelio-3990X) (ClangBuiltLinux clang version 19.1.2 (https://github.com/llvm/llvm-project.git 7ba7d8e2f7b6445b60679da826210cdde29eaf8b), ClangBuiltLinux LLD 19.1.2 (https://github.com/llvm/llvm-project.git 7ba7d8e2f7b6445b60679da826210cdde29eaf8b)) #1 SMP PREEMPT_DYNAMIC Tue Oct 15 19:00:21 MST 2024
  ...
  [    0.631002] Freeing unused kernel image (initmem) memory: 936K
  [    0.631613] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: free_initmem+0x95/0x98
  [    0.632606] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-rc3-00044-g2f87d0916ce0 #1
  [    0.633467] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
  [    0.634583] Call Trace:
  [    0.634837]  panic+0xd4/0x2cc
  [    0.635146]  ? _vdso_rng_data+0xd80/0xd80
  [    0.635550]  ? _vdso_rng_data+0xd80/0xd80
  [    0.635965]  ? rest_init+0xb0/0xb0
  [    0.636312]  __stack_chk_fail+0x10/0x10
  [    0.636701]  ? free_initmem+0x95/0x98
  [    0.637074]  free_initmem+0x95/0x98
  [    0.637434]  ? _vdso_rng_data+0xd80/0xd80
  [    0.637838]  ? rest_init+0xb0/0xb0
  [    0.638196]  kernel_init+0x42/0x1e4
  [    0.638558]  ret_from_fork+0x2b/0x40
  [    0.638922]  ret_from_fork_asm+0x12/0x18
  [    0.639331]  entry_INT80_32+0x108/0x108
  [    0.639864] Kernel Offset: disabled
  [    0.640224] ---[ end Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: free_initmem+0x95/0x98 ]---

I can confirm that this patch resolves that issue for me and LKDTM's
REPORT_STACK_CANARY test passes with that configuration.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>

I presume the '#ifndef CONFIG_X86_64' in arch/x86/entry/entry.S is
present because only X86_32 uses '-mstack-protector-guard-reg='? I
assume that will disappear when X86_64 supports this option (IIRC that
was the plan)?

Cheers,
Nathan
Ard Biesheuvel Oct. 16, 2024, 6:48 a.m. UTC | #4 
On Wed, 16 Oct 2024 at 04:10, Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Tue, Oct 15, 2024 at 12:56:57PM +0200, Ard Biesheuvel wrote:
> > On Mon, 14 Oct 2024 at 22:59, Kees Cook <kees@kernel.org> wrote:
> > >
> > > On Wed, Oct 09, 2024 at 02:43:53PM +0200, Ard Biesheuvel wrote:
> > > > However, if a non-TLS definition of the symbol in question is visible in
> > > > the same compilation unit (which amounts to the whole of vmlinux if LTO
> > > > is enabled), it will drop the per-CPU prefix and emit a load from a
> > > > bogus address.
> > >
> > > I take this to mean that x86 32-bit kernels built with the stack
> > > protector and using Clang LTO will crash very quickly?
> > >
> >
> > Yeah. The linked issue is not quite clear, but it does suggest things
> > are pretty broken in that case.
>
> Yeah, i386_defconfig with CONFIG_LTO_CLANG_FULL=y explodes on boot for
> me without this change:
>
>   [    0.000000] Linux version 6.12.0-rc3-00044-g2f87d0916ce0 (nathan@thelio-3990X) (ClangBuiltLinux clang version 19.1.2 (https://github.com/llvm/llvm-project.git 7ba7d8e2f7b6445b60679da826210cdde29eaf8b), ClangBuiltLinux LLD 19.1.2 (https://github.com/llvm/llvm-project.git 7ba7d8e2f7b6445b60679da826210cdde29eaf8b)) #1 SMP PREEMPT_DYNAMIC Tue Oct 15 19:00:21 MST 2024
>   ...
>   [    0.631002] Freeing unused kernel image (initmem) memory: 936K
>   [    0.631613] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: free_initmem+0x95/0x98
>   [    0.632606] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-rc3-00044-g2f87d0916ce0 #1
>   [    0.633467] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
>   [    0.634583] Call Trace:
>   [    0.634837]  panic+0xd4/0x2cc
>   [    0.635146]  ? _vdso_rng_data+0xd80/0xd80
>   [    0.635550]  ? _vdso_rng_data+0xd80/0xd80
>   [    0.635965]  ? rest_init+0xb0/0xb0
>   [    0.636312]  __stack_chk_fail+0x10/0x10
>   [    0.636701]  ? free_initmem+0x95/0x98
>   [    0.637074]  free_initmem+0x95/0x98
>   [    0.637434]  ? _vdso_rng_data+0xd80/0xd80
>   [    0.637838]  ? rest_init+0xb0/0xb0
>   [    0.638196]  kernel_init+0x42/0x1e4
>   [    0.638558]  ret_from_fork+0x2b/0x40
>   [    0.638922]  ret_from_fork_asm+0x12/0x18
>   [    0.639331]  entry_INT80_32+0x108/0x108
>   [    0.639864] Kernel Offset: disabled
>   [    0.640224] ---[ end Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: free_initmem+0x95/0x98 ]---
>
> I can confirm that this patch resolves that issue for me and LKDTM's
> REPORT_STACK_CANARY test passes with that configuration.
>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
>

Thanks.

> I presume the '#ifndef CONFIG_X86_64' in arch/x86/entry/entry.S is
> present because only X86_32 uses '-mstack-protector-guard-reg='? I
> assume that will disappear when X86_64 supports this option (IIRC that
> was the plan)?
>

Yes, I noticed this issue while enabling
'-mstack-protector-guard-reg=' for x86_64, but i386 is already broken.
diff mbox series

Patch

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index cd75e78a06c1..5b773b34768d 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -142,9 +142,10 @@  ifeq ($(CONFIG_X86_32),y)
 
     ifeq ($(CONFIG_STACKPROTECTOR),y)
         ifeq ($(CONFIG_SMP),y)
-			KBUILD_CFLAGS += -mstack-protector-guard-reg=fs -mstack-protector-guard-symbol=__stack_chk_guard
+            KBUILD_CFLAGS += -mstack-protector-guard-reg=fs \
+                             -mstack-protector-guard-symbol=__ref_stack_chk_guard
         else
-			KBUILD_CFLAGS += -mstack-protector-guard=global
+            KBUILD_CFLAGS += -mstack-protector-guard=global
         endif
     endif
 else
diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S
index d9feadffa972..a503e6d535f8 100644
--- a/arch/x86/entry/entry.S
+++ b/arch/x86/entry/entry.S
@@ -46,3 +46,19 @@  EXPORT_SYMBOL_GPL(mds_verw_sel);
 .popsection
 
 THUNK warn_thunk_thunk, __warn_thunk
+
+#ifndef CONFIG_X86_64
+/*
+ * Clang's implementation of TLS stack cookies requires the variable in
+ * question to be a TLS variable. If the variable happens to be defined as an
+ * ordinary variable with external linkage in the same compilation unit (which
+ * amounts to the whole of vmlinux with LTO enabled), Clang will drop the
+ * segment register prefix from the references, resulting in broken code. Work
+ * around this by avoiding the symbol used in -mstack-protector-guard-symbol=
+ * entirely in the C code, and use an alias emitted by the linker script
+ * instead.
+ */
+#ifdef CONFIG_STACKPROTECTOR
+EXPORT_SYMBOL(__ref_stack_chk_guard);
+#endif
+#endif
diff --git a/arch/x86/include/asm/asm-prototypes.h b/arch/x86/include/asm/asm-prototypes.h
index 25466c4d2134..3674006e3974 100644
--- a/arch/x86/include/asm/asm-prototypes.h
+++ b/arch/x86/include/asm/asm-prototypes.h
@@ -20,3 +20,6 @@ 
 extern void cmpxchg8b_emu(void);
 #endif
 
+#if defined(__GENKSYMS__) && defined(CONFIG_STACKPROTECTOR)
+extern unsigned long __ref_stack_chk_guard;
+#endif
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 07a34d723505..ba83f54dfaa8 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2085,8 +2085,10 @@  void syscall_init(void)
 
 #ifdef CONFIG_STACKPROTECTOR
 DEFINE_PER_CPU(unsigned long, __stack_chk_guard);
+#ifndef CONFIG_SMP
 EXPORT_PER_CPU_SYMBOL(__stack_chk_guard);
 #endif
+#endif
 
 #endif	/* CONFIG_X86_64 */
 
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 2b7c8c14c6fd..a80ad2bf8da4 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -490,6 +490,9 @@  SECTIONS
 . = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
 	   "kernel image bigger than KERNEL_IMAGE_SIZE");
 
+/* needed for Clang - see arch/x86/entry/entry.S */
+PROVIDE(__ref_stack_chk_guard = __stack_chk_guard);
+
 #ifdef CONFIG_X86_64
 /*
  * Per-cpu symbols which need to be offset from __per_cpu_load