Re: [PATCH 6.1 032/772] x86/stackprotector: Work around strict Clang TLS symbol requirements

On Thu, 12 Dec 2024 at 16:54, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:

6.1-stable review patch. If anyone has any objections, please let me know.

---

From: Ard Biesheuvel ardb@kernel.org

[ Upstream commit 577c134d311b9b94598d7a0c86be1f431f823003 ]

There is a follow-up patch that addresses a build issue with this patch and older GCC versions. Given that this fix is for 32-bit only, whereas the build issue affects 64-bit too, let's hold off on this one until we can apply them as a pair.

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 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") Signed-off-by: Ard Biesheuvel ardb@kernel.org Signed-off-by: Brian Gerst brgerst@gmail.com Signed-off-by: Borislav Petkov (AMD) bp@alien8.de Reviewed-by: Nathan Chancellor nathan@kernel.org Tested-by: Nathan Chancellor nathan@kernel.org Cc: stable@vger.kernel.org Link: https://github.com/ClangBuiltLinux/linux/issues/1854 Link: https://lore.kernel.org/r/20241105155801.1779119-2-brgerst@gmail.com Signed-off-by: Sasha Levin sashal@kernel.org

---

arch/x86/Makefile | 3 ++- arch/x86/entry/entry.S | 15 +++++++++++++++ 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, 25 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile index 3419ffa2a3507..a88eede6e7db4 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -113,7 +113,8 @@ 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
            endif
  

diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S index f4419afc7147d..23f9efbe9d705 100644 --- a/arch/x86/entry/entry.S +++ b/arch/x86/entry/entry.S @@ -48,3 +48,18 @@ EXPORT_SYMBOL_GPL(mds_verw_sel);

.popsection

+#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 5cdccea455544..390b13db24b81 100644 --- a/arch/x86/include/asm/asm-prototypes.h +++ b/arch/x86/include/asm/asm-prototypes.h @@ -18,3 +18,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 7f922a359ccc5..b4e999048e9a4 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -2158,8 +2158,10 @@ EXPORT_PER_CPU_SYMBOL(cpu_current_top_of_stack);

#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 78ccb5ec3c0e7..c1e776ed71b06 100644 --- a/arch/x86/kernel/vmlinux.lds.S +++ b/arch/x86/kernel/vmlinux.lds.S @@ -486,6 +486,9 @@ SECTIONS ASSERT(SIZEOF(.rela.dyn) == 0, "Unexpected run-time relocations (.rela) detected!") }

+/* needed for Clang - see arch/x86/entry/entry.S */ +PROVIDE(__ref_stack_chk_guard = __stack_chk_guard);

/*

• The ASSERT() sink to . is intentional, for binutils 2.14 compatibility:

*/

2.43.0