[libc] fix issues around stack protector #74567

SchrodingerZhu · 2023-12-06T06:32:14Z

If a function is declared with stack-protector, the compiler may try to load the TLS. However, inside certain runtime functions, TLS may not be available. This patch disables stack protectors for such routines to fix the problem.

Closes #74487.

If a function is declared with stack-protector, the compiler may try to load the TLS. However, inside the certain runtime functions, TLS may not be available. This patch disables stack protectors for certain routines to fix the problem. Closes llvm#74487.

llvmbot · 2023-12-06T06:32:44Z

@llvm/pr-subscribers-libc

Author: Schrodinger ZHU Yifan (SchrodingerZhu)

Changes

If a function is declared with stack-protector, the compiler may try to load the TLS. However, inside the certain runtime functions, TLS may not be available. This patch disables stack protectors for certain routines to fix the problem.

Closes #74487.

Full diff: https://github.com/llvm/llvm-project/pull/74567.diff

6 Files Affected:

(modified) libc/src/__support/OSUtil/linux/quick_exit.h (+4-2)
(modified) libc/startup/linux/aarch64/CMakeLists.txt (+1)
(modified) libc/startup/linux/aarch64/start.cpp (+3-5)
(modified) libc/startup/linux/riscv/start.cpp (+3-5)
(modified) libc/startup/linux/x86_64/CMakeLists.txt (+1)
(modified) libc/startup/linux/x86_64/start.cpp (+3-5)

diff --git a/libc/src/__support/OSUtil/linux/quick_exit.h b/libc/src/__support/OSUtil/linux/quick_exit.h
index c461d5f8b8fcb..43d9f82784109 100644
--- a/libc/src/__support/OSUtil/linux/quick_exit.h
+++ b/libc/src/__support/OSUtil/linux/quick_exit.h
@@ -9,7 +9,7 @@
 #ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_LINUX_QUICK_EXIT_H
 #define LLVM_LIBC_SRC___SUPPORT_OSUTIL_LINUX_QUICK_EXIT_H
 
-#include "syscall.h"             // For internal syscall function.
+#include "syscall.h" // For internal syscall function.
 
 #include "src/__support/common.h"
 
@@ -17,7 +17,9 @@
 
 namespace LIBC_NAMESPACE {
 
-LIBC_INLINE void quick_exit(int status) {
+// mark as no_stack_protector since TLS can be torn down before calling
+// quick_exit
+__attribute__((no_stack_protector)) LIBC_INLINE void quick_exit(int status) {
   for (;;) {
     LIBC_NAMESPACE::syscall_impl<long>(SYS_exit_group, status);
     LIBC_NAMESPACE::syscall_impl<long>(SYS_exit, status);
diff --git a/libc/startup/linux/aarch64/CMakeLists.txt b/libc/startup/linux/aarch64/CMakeLists.txt
index b47db8eb5d23f..ccd9bbcb9b63d 100644
--- a/libc/startup/linux/aarch64/CMakeLists.txt
+++ b/libc/startup/linux/aarch64/CMakeLists.txt
@@ -13,6 +13,7 @@ add_startup_object(
     libc.src.string.memory_utils.inline_memcpy
     libc.src.unistd.environ
   COMPILE_OPTIONS
+    -fno-stack-protector
     -fno-omit-frame-pointer
     -ffreestanding # To avoid compiler warnings about calling the main function.
 )
diff --git a/libc/startup/linux/aarch64/start.cpp b/libc/startup/linux/aarch64/start.cpp
index b5c426866b56d..858995203d9b2 100644
--- a/libc/startup/linux/aarch64/start.cpp
+++ b/libc/startup/linux/aarch64/start.cpp
@@ -184,7 +184,7 @@ __attribute__((noinline)) static void do_start() {
     app.tls.align = phdr->p_align;
   }
 
-  LIBC_NAMESPACE::TLSDescriptor tls;
+  static LIBC_NAMESPACE::TLSDescriptor tls;
   LIBC_NAMESPACE::init_tls(tls);
   if (tls.size != 0)
     LIBC_NAMESPACE::set_thread_ptr(tls.tp);
@@ -193,6 +193,8 @@ __attribute__((noinline)) static void do_start() {
   LIBC_NAMESPACE::main_thread_attrib.atexit_callback_mgr =
       LIBC_NAMESPACE::internal::get_thread_atexit_callback_mgr();
 
+  LIBC_NAMESPACE::atexit(
+      []() { LIBC_NAMESPACE::cleanup_tls(tls.tp, tls.size); });
   // We want the fini array callbacks to be run after other atexit
   // callbacks are run. So, we register them before running the init
   // array callbacks as they can potentially register their own atexit
@@ -208,10 +210,6 @@ __attribute__((noinline)) static void do_start() {
                     reinterpret_cast<char **>(app.args->argv),
                     reinterpret_cast<char **>(env_ptr));
 
-  // TODO: TLS cleanup should be done after all other atexit callbacks
-  // are run. So, register a cleanup callback for it with atexit before
-  // everything else.
-  LIBC_NAMESPACE::cleanup_tls(tls.addr, tls.size);
   LIBC_NAMESPACE::exit(retval);
 }
 
diff --git a/libc/startup/linux/riscv/start.cpp b/libc/startup/linux/riscv/start.cpp
index bf04be5ad14ad..8bd76631f402c 100644
--- a/libc/startup/linux/riscv/start.cpp
+++ b/libc/startup/linux/riscv/start.cpp
@@ -187,7 +187,7 @@ __attribute__((noinline)) static void do_start() {
     app.tls.align = phdr->p_align;
   }
 
-  LIBC_NAMESPACE::TLSDescriptor tls;
+  static LIBC_NAMESPACE::TLSDescriptor tls;
   LIBC_NAMESPACE::init_tls(tls);
   if (tls.size != 0)
     LIBC_NAMESPACE::set_thread_ptr(tls.tp);
@@ -196,6 +196,8 @@ __attribute__((noinline)) static void do_start() {
   LIBC_NAMESPACE::main_thread_attrib.atexit_callback_mgr =
       LIBC_NAMESPACE::internal::get_thread_atexit_callback_mgr();
 
+  LIBC_NAMESPACE::atexit(
+      []() { LIBC_NAMESPACE::cleanup_tls(tls.tp, tls.size); });
   // We want the fini array callbacks to be run after other atexit
   // callbacks are run. So, we register them before running the init
   // array callbacks as they can potentially register their own atexit
@@ -211,10 +213,6 @@ __attribute__((noinline)) static void do_start() {
                     reinterpret_cast<char **>(app.args->argv),
                     reinterpret_cast<char **>(env_ptr));
 
-  // TODO: TLS cleanup should be done after all other atexit callbacks
-  // are run. So, register a cleanup callback for it with atexit before
-  // everything else.
-  LIBC_NAMESPACE::cleanup_tls(tls.addr, tls.size);
   LIBC_NAMESPACE::exit(retval);
 }
 
diff --git a/libc/startup/linux/x86_64/CMakeLists.txt b/libc/startup/linux/x86_64/CMakeLists.txt
index 076c0c3e444f5..aac5a0626a176 100644
--- a/libc/startup/linux/x86_64/CMakeLists.txt
+++ b/libc/startup/linux/x86_64/CMakeLists.txt
@@ -15,6 +15,7 @@ add_startup_object(
     libc.src.string.memory_utils.inline_memcpy
     libc.src.unistd.environ
   COMPILE_OPTIONS
+    -fno-stack-protector
     -fno-omit-frame-pointer
     -ffreestanding # To avoid compiler warnings about calling the main function.
     -fno-builtin
diff --git a/libc/startup/linux/x86_64/start.cpp b/libc/startup/linux/x86_64/start.cpp
index bc1b4f0487f31..96031723e009a 100644
--- a/libc/startup/linux/x86_64/start.cpp
+++ b/libc/startup/linux/x86_64/start.cpp
@@ -222,7 +222,7 @@ extern "C" void _start() {
     app.tls.align = phdr->p_align;
   }
 
-  LIBC_NAMESPACE::TLSDescriptor tls;
+  static LIBC_NAMESPACE::TLSDescriptor tls;
   LIBC_NAMESPACE::init_tls(tls);
   if (tls.size != 0 && !LIBC_NAMESPACE::set_thread_ptr(tls.tp))
     LIBC_NAMESPACE::syscall_impl<long>(SYS_exit, 1);
@@ -231,6 +231,8 @@ extern "C" void _start() {
   LIBC_NAMESPACE::main_thread_attrib.atexit_callback_mgr =
       LIBC_NAMESPACE::internal::get_thread_atexit_callback_mgr();
 
+  LIBC_NAMESPACE::atexit(
+      []() { LIBC_NAMESPACE::cleanup_tls(tls.tp, tls.size); });
   // We want the fini array callbacks to be run after other atexit
   // callbacks are run. So, we register them before running the init
   // array callbacks as they can potentially register their own atexit
@@ -246,9 +248,5 @@ extern "C" void _start() {
                     reinterpret_cast<char **>(app.args->argv),
                     reinterpret_cast<char **>(env_ptr));
 
-  // TODO: TLS cleanup should be done after all other atexit callbacks
-  // are run. So, register a cleanup callback for it with atexit before
-  // everything else.
-  LIBC_NAMESPACE::cleanup_tls(tls.addr, tls.size);
   LIBC_NAMESPACE::exit(retval);
 }

libc/startup/linux/aarch64/start.cpp

libc/startup/linux/x86_64/CMakeLists.txt

libc/src/__support/OSUtil/linux/quick_exit.h

nickdesaulniers · 2023-12-06T19:53:44Z

I thought certain platforms have a global vs thread local stack canary?

SchrodingerZhu · 2023-12-06T20:25:39Z

I thought certain platforms have a global vs thread local stack canary?

Could you please give a link to the specifications? I wish I can add links in the comment and gate the attributes accordingly.

SchrodingerZhu · 2023-12-06T20:54:11Z

@nickdesaulniers it does not seem to be arch dependent on linux: https://github.com/bminor/musl/blob/f314e133929b6379eccc632bef32eaebb66a7335/src/env/__stack_chk_fail.c#L21C6-L21C6

Update
Never mind. I was wrong according to https://godbolt.org/z/ndbr9qGPz. MUSL is just using an arch-independent way.
It would be great if we can find a summary of these behaviors somewhere.

SchrodingerZhu · 2023-12-07T16:15:37Z

I updated the patch to disable stack protector explicitly for x86-64. The atexit changes are still applied to all archs since 1) it is more consistent 2) it may still be possible for other callbacks to use TLS for purposes other than stack protector.

nickdesaulniers · 2023-12-07T16:40:56Z

I thought certain platforms have a global vs thread local stack canary?

Could you please give a link to the specifications? I wish I can add links in the comment and gate the attributes accordingly.

This is only from observation; my prior work on the Linux kernel involved getting additional functionality added to clang for the kernel's more exotic stack protector implementations. The kernel doesn't share the same ABI as userspace, so it can break its own internal ABI whenever, which allows for it to do better than the ABI constrained userspace.

I'd guess maybe the Itanium x86 psABI, ARM ARM, and RISCV psABI might have specific details, but I don't know. I've only observed behavior by playing with the compiler.

SchrodingerZhu · 2023-12-08T03:48:52Z

I think we are doing the right thing.

For documentation, I suggest us reading

llvm-project/llvm/lib/Target/X86/X86ISelLoweringCall.cpp

Line 666 in 5fc76e6

X86TargetLowering::getSafeStackPointerLocation(IRBuilderBase &IRB) const {

and all its default/override implementations.

libc/src/__support/OSUtil/linux/quick_exit.h

llvmbot added the libc label Dec 6, 2023

michaelrj-google reviewed Dec 6, 2023

View reviewed changes

libc/startup/linux/aarch64/start.cpp Show resolved Hide resolved

libc/startup/linux/aarch64/start.cpp Show resolved Hide resolved

libc/startup/linux/x86_64/CMakeLists.txt Show resolved Hide resolved

nickdesaulniers reviewed Dec 6, 2023

View reviewed changes

libc/src/__support/OSUtil/linux/quick_exit.h Outdated Show resolved Hide resolved

address CR: add comments

387cc2d

address CR: only disable stack protector for x86

2d2d699

nickdesaulniers reviewed Dec 12, 2023

View reviewed changes

libc/src/__support/OSUtil/linux/quick_exit.h Show resolved Hide resolved

nickdesaulniers approved these changes Dec 12, 2023

View reviewed changes

simplify x86 detection

a541371

nickdesaulniers merged commit 3568521 into llvm:main Dec 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc] fix issues around stack protector #74567

[libc] fix issues around stack protector #74567

Uh oh!

SchrodingerZhu commented Dec 6, 2023 •

edited

Loading

Uh oh!

llvmbot commented Dec 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickdesaulniers commented Dec 6, 2023

Uh oh!

SchrodingerZhu commented Dec 6, 2023

Uh oh!

SchrodingerZhu commented Dec 6, 2023 •

edited

Loading

Uh oh!

SchrodingerZhu commented Dec 7, 2023

Uh oh!

nickdesaulniers commented Dec 7, 2023 •

edited

Loading

Uh oh!

SchrodingerZhu commented Dec 8, 2023

Uh oh!

Uh oh!

Uh oh!

[libc] fix issues around stack protector #74567

[libc] fix issues around stack protector #74567

Uh oh!

Conversation

SchrodingerZhu commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickdesaulniers commented Dec 6, 2023

Uh oh!

SchrodingerZhu commented Dec 6, 2023

Uh oh!

SchrodingerZhu commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SchrodingerZhu commented Dec 7, 2023

Uh oh!

nickdesaulniers commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SchrodingerZhu commented Dec 8, 2023

Uh oh!

Uh oh!

Uh oh!

SchrodingerZhu commented Dec 6, 2023 •

edited

Loading

SchrodingerZhu commented Dec 6, 2023 •

edited

Loading

nickdesaulniers commented Dec 7, 2023 •

edited

Loading