On Sat, 5 Oct 2024 at 17:48, Brian Gerst brgerst@gmail.com wrote:
On Wed, Oct 2, 2024 at 7:04 AM Ard Biesheuvel ardb@kernel.org wrote:
On Wed, 2 Oct 2024 at 11:25, Ard Biesheuvel ardb+git@google.com wrote:
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
arch/x86/Makefile | 5 +++-- arch/x86/entry/entry.S | 16 ++++++++++++++++ arch/x86/kernel/cpu/common.c | 2 ++ arch/x86/kernel/vmlinux.lds.S | 3 +++ 4 files changed, 24 insertions(+), 2 deletions(-)
This needs the hunk below applied on top for CONFIG_MODVERSIONS:
--- 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
+#ifdef CONFIG_STACKPROTECTOR +extern unsigned long __ref_stack_chk_guard; +#endif
Shouldn't this also be guarded by __GENKSYMS__, since the whole point of this is to hide the declaration from the compiler?
Yes, good point. Even though it does not matter in practice (the issue is tickled only by a visible *definition*, not by a declaration), this file is included into C code, which should be avoided.