From d16c5b904854c2bce0652b63bc2f63fbc455a905 Mon Sep 17 00:00:00 2001 From: Flavio Ceolin Date: Tue, 1 Aug 2023 15:07:57 -0700 Subject: [PATCH] kernel: canaries: Allow using TLS to store it Add new option to use thread local storage for stack canaries. This makes harder to find the canaries location and value. This is made optional because there is a performance and size penalty when using it. Signed-off-by: Flavio Ceolin --- arch/Kconfig | 4 ++++ cmake/compiler/gcc/compiler_flags.cmake | 7 ++++++- include/zephyr/sys/libc-hooks.h | 3 ++- kernel/Kconfig | 18 ++++++++++++++++++ kernel/compiler_stack_protect.c | 4 +++- kernel/init.c | 4 ++++ kernel/xip.c | 4 ++++ lib/os/thread_entry.c | 14 +++++++++++++- 8 files changed, 54 insertions(+), 4 deletions(-) diff --git a/arch/Kconfig b/arch/Kconfig index afa6fb0a709..332bb49455f 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -92,6 +92,7 @@ config X86 select NEED_LIBC_MEM_PARTITION if USERSPACE && TIMING_FUNCTIONS \ && !BOARD_HAS_TIMING_FUNCTIONS \ && !SOC_HAS_TIMING_FUNCTIONS + select ARCH_HAS_STACK_CANARIES_TLS help x86 architecture @@ -600,6 +601,9 @@ config ARCH_HAS_SUSPEND_TO_RAM help When selected, the architecture supports suspend-to-RAM (S2RAM). +config ARCH_HAS_STACK_CANARIES_TLS + bool + # # Other architecture related options # diff --git a/cmake/compiler/gcc/compiler_flags.cmake b/cmake/compiler/gcc/compiler_flags.cmake index 3283c0d86a6..741d49074f6 100644 --- a/cmake/compiler/gcc/compiler_flags.cmake +++ b/cmake/compiler/gcc/compiler_flags.cmake @@ -160,7 +160,12 @@ set_compiler_property(PROPERTY coverage -fprofile-arcs -ftest-coverage -fno-inli set_compiler_property(PROPERTY security_canaries -fstack-protector-all) # Only a valid option with GCC 7.x and above, so let's do check and set. -check_set_compiler_property(APPEND PROPERTY security_canaries -mstack-protector-guard=global) +if(CONFIG_STACK_CANARIES_TLS) + check_set_compiler_property(APPEND PROPERTY security_canaries -mstack-protector-guard=tls) +else() + check_set_compiler_property(APPEND PROPERTY security_canaries -mstack-protector-guard=global) +endif() + if(NOT CONFIG_NO_OPTIMIZATIONS) # _FORTIFY_SOURCE: Detect common-case buffer overflows for certain functions diff --git a/include/zephyr/sys/libc-hooks.h b/include/zephyr/sys/libc-hooks.h index 10d498419a2..d1a1afcbf40 100644 --- a/include/zephyr/sys/libc-hooks.h +++ b/include/zephyr/sys/libc-hooks.h @@ -92,7 +92,8 @@ __syscall size_t zephyr_fwrite(const void *ZRESTRICT ptr, size_t size, extern struct k_mem_partition z_malloc_partition; #endif -#if defined(CONFIG_NEWLIB_LIBC) || defined(CONFIG_STACK_CANARIES) || \ +#if defined(CONFIG_NEWLIB_LIBC) || (defined(CONFIG_STACK_CANARIES) && \ + !defined(CONFIG_STACK_CANARIES_TLS)) || \ defined(CONFIG_PICOLIBC) || defined(CONFIG_NEED_LIBC_MEM_PARTITION) /* - All newlib globals will be placed into z_libc_partition. * - Minimal C library globals, if any, will be placed into diff --git a/kernel/Kconfig b/kernel/Kconfig index d92aff41f7b..779ccdaee33 100644 --- a/kernel/Kconfig +++ b/kernel/Kconfig @@ -836,6 +836,24 @@ config STACK_CANARIES If stack canaries are not supported by the compiler an error will occur at build time. +if STACK_CANARIES + +config STACK_CANARIES_TLS + bool "Stack canaries using thread local storage" + depends on THREAD_LOCAL_STORAGE + depends on ARCH_HAS_STACK_CANARIES_TLS + help + This option enables compiler stack canaries on TLS. + + Stack canaries will leave in the thread local storage and + each thread will have its own canary. This makes harder + to predict the canary location and value. + + When enabled this causes an additional performance penalty + during thread creations because it needs a new random value + per thread. +endif + config EXECUTE_XOR_WRITE bool "W^X for memory partitions" depends on USERSPACE diff --git a/kernel/compiler_stack_protect.c b/kernel/compiler_stack_protect.c index 790840c4324..2ee2d81c7a3 100644 --- a/kernel/compiler_stack_protect.c +++ b/kernel/compiler_stack_protect.c @@ -46,7 +46,9 @@ void _StackCheckHandler(void) * Symbol referenced by GCC compiler generated code for canary value. * The canary value gets initialized in z_cstart(). */ -#ifdef CONFIG_USERSPACE +#ifdef CONFIG_STACK_CANARIES_TLS +__thread uintptr_t __stack_chk_guard; +#elif CONFIG_USERSPACE K_APP_DMEM(z_libc_partition) uintptr_t __stack_chk_guard; #else __noinit uintptr_t __stack_chk_guard; diff --git a/kernel/init.c b/kernel/init.c index 9c4cbce9539..3d5991f1fbe 100644 --- a/kernel/init.c +++ b/kernel/init.c @@ -217,7 +217,11 @@ void z_bss_zero_pinned(void) #endif /* CONFIG_LINKER_USE_PINNED_SECTION */ #ifdef CONFIG_STACK_CANARIES +#ifdef CONFIG_STACK_CANARIES_TLS +extern __thread volatile uintptr_t __stack_chk_guard; +#else extern volatile uintptr_t __stack_chk_guard; +#endif #endif /* CONFIG_STACK_CANARIES */ /* LCOV_EXCL_STOP */ diff --git a/kernel/xip.c b/kernel/xip.c index e1afa325dff..a5752560c85 100644 --- a/kernel/xip.c +++ b/kernel/xip.c @@ -11,7 +11,11 @@ #include #ifdef CONFIG_STACK_CANARIES +#ifdef CONFIG_STACK_CANARIES_TLS +extern __thread volatile uintptr_t __stack_chk_guard; +#else extern volatile uintptr_t __stack_chk_guard; +#endif /* CONFIG_STACK_CANARIES_TLS */ #endif /* CONFIG_STACK_CANARIES */ /** diff --git a/lib/os/thread_entry.c b/lib/os/thread_entry.c index 40565d64ec4..0add601489c 100644 --- a/lib/os/thread_entry.c +++ b/lib/os/thread_entry.c @@ -12,11 +12,16 @@ */ #include - #ifdef CONFIG_THREAD_LOCAL_STORAGE +#include + __thread k_tid_t z_tls_current; #endif +#ifdef CONFIG_STACK_CANARIES_TLS +extern __thread volatile uintptr_t __stack_chk_guard; +#endif /* CONFIG_STACK_CANARIES_TLS */ + /* * Common thread entry point function (used by all threads) * @@ -33,6 +38,13 @@ FUNC_NORETURN void z_thread_entry(k_thread_entry_t entry, #ifdef CONFIG_THREAD_LOCAL_STORAGE z_tls_current = z_current_get(); #endif +#ifdef CONFIG_STACK_CANARIES_TLS + uintptr_t stack_guard; + + sys_rand_get((uint8_t *)&stack_guard, sizeof(stack_guard)); + __stack_chk_guard = stack_guard; + __stack_chk_guard <<= 8; +#endif /* CONFIG_STACK_CANARIES */ entry(p1, p2, p3); k_thread_abort(k_current_get());