[libc-commits] [libc] [libc] fix issues around stack protector (PR #74567)
Schrodinger ZHU Yifan via libc-commits
libc-commits at lists.llvm.org
Tue Dec 12 12:14:09 PST 2023
https://github.com/SchrodingerZhu updated https://github.com/llvm/llvm-project/pull/74567
>From 3ded6ce677220bbac2fff3bd1178fc90dd0942a7 Mon Sep 17 00:00:00 2001
From: Schrodinger ZHU Yifan <yifanzhu at rochester.edu>
Date: Wed, 6 Dec 2023 01:26:59 -0500
Subject: [PATCH 1/4] [libc] fix issues around stack protector
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.
---
libc/src/__support/OSUtil/linux/quick_exit.h | 6 ++++--
libc/startup/linux/aarch64/CMakeLists.txt | 1 +
libc/startup/linux/aarch64/start.cpp | 8 +++-----
libc/startup/linux/riscv/start.cpp | 8 +++-----
libc/startup/linux/x86_64/CMakeLists.txt | 1 +
libc/startup/linux/x86_64/start.cpp | 8 +++-----
6 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/libc/src/__support/OSUtil/linux/quick_exit.h b/libc/src/__support/OSUtil/linux/quick_exit.h
index c461d5f8b8fcb6..43d9f827841096 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 b47db8eb5d23f3..ccd9bbcb9b63d5 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 b5c426866b56d7..858995203d9b2b 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 bf04be5ad14ad1..8bd76631f402cd 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 076c0c3e444f56..aac5a0626a176a 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 bc1b4f0487f316..96031723e009a9 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);
}
>From 387cc2dc33bda9414ed6fb5efa8a5160c51d0ce0 Mon Sep 17 00:00:00 2001
From: Schrodinger ZHU Yifan <yifanzhu at rochester.edu>
Date: Wed, 6 Dec 2023 15:36:44 -0500
Subject: [PATCH 2/4] address CR: add comments
---
libc/startup/linux/aarch64/start.cpp | 6 +++++-
libc/startup/linux/riscv/start.cpp | 6 +++++-
libc/startup/linux/x86_64/start.cpp | 6 +++++-
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/libc/startup/linux/aarch64/start.cpp b/libc/startup/linux/aarch64/start.cpp
index 858995203d9b2b..c3e20eb09e4b42 100644
--- a/libc/startup/linux/aarch64/start.cpp
+++ b/libc/startup/linux/aarch64/start.cpp
@@ -184,6 +184,8 @@ __attribute__((noinline)) static void do_start() {
app.tls.align = phdr->p_align;
}
+ // This descriptor has to be static since its cleanup function cannot
+ // capture the context.
static LIBC_NAMESPACE::TLSDescriptor tls;
LIBC_NAMESPACE::init_tls(tls);
if (tls.size != 0)
@@ -192,7 +194,9 @@ __attribute__((noinline)) static void do_start() {
LIBC_NAMESPACE::self.attrib = &LIBC_NAMESPACE::main_thread_attrib;
LIBC_NAMESPACE::main_thread_attrib.atexit_callback_mgr =
LIBC_NAMESPACE::internal::get_thread_atexit_callback_mgr();
-
+ // We register the cleanup_tls function to be the last atexit callback to be
+ // invoked. It will tear down the TLS. Other callbacks may depend on TLS (such
+ // as the stack protector canary).
LIBC_NAMESPACE::atexit(
[]() { LIBC_NAMESPACE::cleanup_tls(tls.tp, tls.size); });
// We want the fini array callbacks to be run after other atexit
diff --git a/libc/startup/linux/riscv/start.cpp b/libc/startup/linux/riscv/start.cpp
index 8bd76631f402cd..4d37662ccea13c 100644
--- a/libc/startup/linux/riscv/start.cpp
+++ b/libc/startup/linux/riscv/start.cpp
@@ -187,6 +187,8 @@ __attribute__((noinline)) static void do_start() {
app.tls.align = phdr->p_align;
}
+ // This descriptor has to be static since its cleanup function cannot
+ // capture the context.
static LIBC_NAMESPACE::TLSDescriptor tls;
LIBC_NAMESPACE::init_tls(tls);
if (tls.size != 0)
@@ -195,7 +197,9 @@ __attribute__((noinline)) static void do_start() {
LIBC_NAMESPACE::self.attrib = &LIBC_NAMESPACE::main_thread_attrib;
LIBC_NAMESPACE::main_thread_attrib.atexit_callback_mgr =
LIBC_NAMESPACE::internal::get_thread_atexit_callback_mgr();
-
+ // We register the cleanup_tls function to be the last atexit callback to be
+ // invoked. It will tear down the TLS. Other callbacks may depend on TLS (such
+ // as the stack protector canary).
LIBC_NAMESPACE::atexit(
[]() { LIBC_NAMESPACE::cleanup_tls(tls.tp, tls.size); });
// We want the fini array callbacks to be run after other atexit
diff --git a/libc/startup/linux/x86_64/start.cpp b/libc/startup/linux/x86_64/start.cpp
index 96031723e009a9..496105dfd0b43a 100644
--- a/libc/startup/linux/x86_64/start.cpp
+++ b/libc/startup/linux/x86_64/start.cpp
@@ -222,6 +222,8 @@ extern "C" void _start() {
app.tls.align = phdr->p_align;
}
+ // This descriptor has to be static since its cleanup function cannot
+ // capture the context.
static LIBC_NAMESPACE::TLSDescriptor tls;
LIBC_NAMESPACE::init_tls(tls);
if (tls.size != 0 && !LIBC_NAMESPACE::set_thread_ptr(tls.tp))
@@ -230,7 +232,9 @@ extern "C" void _start() {
LIBC_NAMESPACE::self.attrib = &LIBC_NAMESPACE::main_thread_attrib;
LIBC_NAMESPACE::main_thread_attrib.atexit_callback_mgr =
LIBC_NAMESPACE::internal::get_thread_atexit_callback_mgr();
-
+ // We register the cleanup_tls function to be the last atexit callback to be
+ // invoked. It will tear down the TLS. Other callbacks may depend on TLS (such
+ // as the stack protector canary).
LIBC_NAMESPACE::atexit(
[]() { LIBC_NAMESPACE::cleanup_tls(tls.tp, tls.size); });
// We want the fini array callbacks to be run after other atexit
>From 2d2d699cf981239b7486c1379cb135531d4c2b13 Mon Sep 17 00:00:00 2001
From: Schrodinger ZHU Yifan <yifanzhu at rochester.edu>
Date: Thu, 7 Dec 2023 11:12:08 -0500
Subject: [PATCH 3/4] address CR: only disable stack protector for x86
---
libc/src/__support/OSUtil/linux/quick_exit.h | 11 ++++++++---
libc/startup/linux/aarch64/CMakeLists.txt | 1 -
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/libc/src/__support/OSUtil/linux/quick_exit.h b/libc/src/__support/OSUtil/linux/quick_exit.h
index 43d9f827841096..356fc1136e8ab6 100644
--- a/libc/src/__support/OSUtil/linux/quick_exit.h
+++ b/libc/src/__support/OSUtil/linux/quick_exit.h
@@ -17,9 +17,14 @@
namespace LIBC_NAMESPACE {
-// 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) {
+// mark as no_stack_protector for x86 since TLS can be torn down before calling
+// quick_exit so that the stack protector canary cannot be loaded.
+#if defined(__i386) || defined(__i386__) || defined(__x86_64__) || \
+ defined(__x86_64)
+__attribute__((no_stack_protector))
+#endif
+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 ccd9bbcb9b63d5..b47db8eb5d23f3 100644
--- a/libc/startup/linux/aarch64/CMakeLists.txt
+++ b/libc/startup/linux/aarch64/CMakeLists.txt
@@ -13,7 +13,6 @@ 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.
)
>From a541371fe54815fd479bd2013f033f03f4a60c93 Mon Sep 17 00:00:00 2001
From: Schrodinger ZHU Yifan <yifanzhu at rochester.edu>
Date: Tue, 12 Dec 2023 15:13:53 -0500
Subject: [PATCH 4/4] simplify x86 detection
---
libc/src/__support/OSUtil/linux/quick_exit.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/libc/src/__support/OSUtil/linux/quick_exit.h b/libc/src/__support/OSUtil/linux/quick_exit.h
index 356fc1136e8ab6..432395584d8467 100644
--- a/libc/src/__support/OSUtil/linux/quick_exit.h
+++ b/libc/src/__support/OSUtil/linux/quick_exit.h
@@ -19,8 +19,7 @@ namespace LIBC_NAMESPACE {
// mark as no_stack_protector for x86 since TLS can be torn down before calling
// quick_exit so that the stack protector canary cannot be loaded.
-#if defined(__i386) || defined(__i386__) || defined(__x86_64__) || \
- defined(__x86_64)
+#ifdef LIBC_TARGET_ARCH_IS_X86
__attribute__((no_stack_protector))
#endif
LIBC_INLINE void
More information about the libc-commits
mailing list