[libc] Simple __stack_chk_guard implementation #78804
Conversation
This is needed on systems that don't use TLS to store the stack canary.
|
@llvm/pr-subscribers-libc Author: Petr Hosek (petrhosek) ChangesThis is needed on systems that don't use TLS to store the stack canary. Full diff: https://github.com/llvm/llvm-project/pull/78804.diff 11 Files Affected:
diff --git a/libc/config/baremetal/arm/entrypoints.txt b/libc/config/baremetal/arm/entrypoints.txt
index f725b1c2394c6a3..39f54440780deb8 100644
--- a/libc/config/baremetal/arm/entrypoints.txt
+++ b/libc/config/baremetal/arm/entrypoints.txt
@@ -19,6 +19,7 @@ set(TARGET_LIBC_ENTRYPOINTS
# compiler entrypoints (no corresponding header)
libc.src.compiler.__stack_chk_fail
+ libc.src.compiler.__stack_chk_guard
# errno.h entrypoints
libc.src.errno.errno
diff --git a/libc/config/baremetal/riscv/entrypoints.txt b/libc/config/baremetal/riscv/entrypoints.txt
index 5da755170fda96e..79bd24bea103aea 100644
--- a/libc/config/baremetal/riscv/entrypoints.txt
+++ b/libc/config/baremetal/riscv/entrypoints.txt
@@ -19,6 +19,7 @@ set(TARGET_LIBC_ENTRYPOINTS
# compiler entrypoints (no corresponding header)
libc.src.compiler.__stack_chk_fail
+ libc.src.compiler.__stack_chk_guard
# errno.h entrypoints
libc.src.errno.errno
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index 625fa6bffe63c65..7496947a781bd4f 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -366,6 +366,7 @@ if(LLVM_LIBC_FULL_BUILD)
list(APPEND TARGET_LIBC_ENTRYPOINTS
# compiler entrypoints (no corresponding header)
libc.src.compiler.__stack_chk_fail
+ libc.src.compiler.__stack_chk_guard
# network.h entrypoints
libc.src.network.htonl
diff --git a/libc/config/linux/arm/entrypoints.txt b/libc/config/linux/arm/entrypoints.txt
index c75ac2302d4ac45..7070f6c4e5a1521 100644
--- a/libc/config/linux/arm/entrypoints.txt
+++ b/libc/config/linux/arm/entrypoints.txt
@@ -235,6 +235,14 @@ set(TARGET_LIBM_ENTRYPOINTS
libc.src.math.truncl
)
+if(LLVM_LIBC_FULL_BUILD)
+ list(APPEND TARGET_LIBC_ENTRYPOINTS
+ # compiler entrypoints (no corresponding header)
+ libc.src.compiler.__stack_chk_fail
+ libc.src.compiler.__stack_chk_guard
+ )
+endif()
+
set(TARGET_LLVMLIBC_ENTRYPOINTS
${TARGET_LIBC_ENTRYPOINTS}
${TARGET_LIBM_ENTRYPOINTS}
diff --git a/libc/src/compiler/CMakeLists.txt b/libc/src/compiler/CMakeLists.txt
index aa59d84e08d1467..b37b2dbe1c61989 100644
--- a/libc/src/compiler/CMakeLists.txt
+++ b/libc/src/compiler/CMakeLists.txt
@@ -1,9 +1,9 @@
if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
-else()
- add_subdirectory(generic)
endif()
+add_subdirectory(generic)
+
if(TARGET libc.src.compiler.${LIBC_TARGET_OS}.__stack_chk_fail)
set(stack_chk_fail_dep libc.src.compiler.${LIBC_TARGET_OS}.__stack_chk_fail)
else()
@@ -16,3 +16,16 @@ add_entrypoint_object(
DEPENDS
${stack_chk_fail_dep}
)
+
+if(TARGET libc.src.compiler.${LIBC_TARGET_OS}.__stack_chk_guard)
+ set(stack_chk_guard_dep libc.src.compiler.${LIBC_TARGET_OS}.__stack_chk_guard)
+else()
+ set(stack_chk_guard_dep libc.src.compiler.generic.__stack_chk_guard)
+endif()
+
+add_entrypoint_object(
+ __stack_chk_guard
+ ALIAS
+ DEPENDS
+ ${stack_chk_guard_dep}
+)
diff --git a/libc/src/compiler/__stack_chk_guard.h b/libc/src/compiler/__stack_chk_guard.h
new file mode 100644
index 000000000000000..17edefa3465d7e5
--- /dev/null
+++ b/libc/src/compiler/__stack_chk_guard.h
@@ -0,0 +1,20 @@
+//===-- Internal header for __stack_chk_guard -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_COMPILER___STACK_CHK_GUARD_H
+#define LLVM_LIBC_SRC_COMPILER___STACK_CHK_GUARD_H
+
+#include <stdint.h>
+
+// The compiler will emit calls implicitly to a non-namespaced version.
+// TODO: can we additionally provide a namespaced alias so that tests can
+// explicitly call the namespaced variant rather than the non-namespaced
+// definition?
+extern "C" uintptr_t __stack_chk_guard;
+
+#endif // LLVM_LIBC_SRC_COMPILER___STACK_CHK_GUARD_H
diff --git a/libc/src/compiler/baremetal/CMakeLists.txt b/libc/src/compiler/baremetal/CMakeLists.txt
new file mode 100644
index 000000000000000..222ca9fc1523671
--- /dev/null
+++ b/libc/src/compiler/baremetal/CMakeLists.txt
@@ -0,0 +1,7 @@
+add_entrypoint_object(
+ __stack_chk_guard
+ SRCS
+ __stack_chk_guard.cpp
+ HDRS
+ ../__stack_chk_guard.h
+)
diff --git a/libc/src/compiler/baremetal/__stack_chk_guard.cpp b/libc/src/compiler/baremetal/__stack_chk_guard.cpp
new file mode 100644
index 000000000000000..691e0c4f725220f
--- /dev/null
+++ b/libc/src/compiler/baremetal/__stack_chk_guard.cpp
@@ -0,0 +1,17 @@
+//===-- Implementation of __stack_chk_guard -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/compiler/__stack_chk_guard.h"
+
+#include <stdint.h>
+
+extern "C" {
+
+uintptr_t __stack_chk_guard = 0x00000aff; // 0, 0, '\n', 255
+
+} // extern "C"
diff --git a/libc/src/compiler/linux/CMakeLists.txt b/libc/src/compiler/linux/CMakeLists.txt
new file mode 100644
index 000000000000000..33918fb2f4db28c
--- /dev/null
+++ b/libc/src/compiler/linux/CMakeLists.txt
@@ -0,0 +1,11 @@
+add_entrypoint_object(
+ __stack_chk_guard
+ SRCS
+ __stack_chk_guard.cpp
+ HDRS
+ ../__stack_chk_guard.h
+ DEPENDS
+ libc.src.__support.OSUtil.osutil
+ COMPILE_OPTIONS
+ -Wno-global-constructors
+)
diff --git a/libc/src/compiler/linux/__stack_chk_guard.cpp b/libc/src/compiler/linux/__stack_chk_guard.cpp
new file mode 100644
index 000000000000000..112dd2a52da70cd
--- /dev/null
+++ b/libc/src/compiler/linux/__stack_chk_guard.cpp
@@ -0,0 +1,39 @@
+//===-- Implementation of __stack_chk_guard -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/compiler/__stack_chk_guard.h"
+#include "src/__support/OSUtil/syscall.h"
+#include "src/errno/libc_errno.h"
+
+#include <sys/syscall.h>
+
+extern "C" {
+
+uintptr_t __stack_chk_guard = 0;
+
+} // extern "C"
+
+namespace LIBC_NAMESPACE {
+namespace {
+
+class StackCheckGuard {
+public:
+ StackCheckGuard() {
+ // TODO: Use getauxval(AT_RANDOM) once available.
+ long retval =
+ syscall_impl(SYS_getrandom, reinterpret_cast<long>(&__stack_chk_guard),
+ sizeof(uintptr_t), 0);
+ if (retval < 0)
+ __stack_chk_guard = 0x00000aff; // 0, 0, '\n', 255
+ }
+};
+
+StackCheckGuard stack_check_guard;
+
+} // anonymous namespace
+} // namespace LIBC_NAMESPACE
diff --git a/libc/test/src/compiler/CMakeLists.txt b/libc/test/src/compiler/CMakeLists.txt
index 65a9acceb6f7f10..39f96eb2272431a 100644
--- a/libc/test/src/compiler/CMakeLists.txt
+++ b/libc/test/src/compiler/CMakeLists.txt
@@ -9,6 +9,7 @@ add_libc_unittest(
DEPENDS
libc.src.__support.macros.sanitizer
libc.src.compiler.__stack_chk_fail
+ libc.src.compiler.__stack_chk_guard
libc.src.string.memset
COMPILE_OPTIONS
-fstack-protector-all
|
| @@ -19,6 +19,7 @@ set(TARGET_LIBC_ENTRYPOINTS | |||
|
|
|||
| # compiler entrypoints (no corresponding header) | |||
| libc.src.compiler.__stack_chk_fail | |||
| libc.src.compiler.__stack_chk_guard | |||
There was a problem hiding this comment.
should this also be enabled for x86_64?
There was a problem hiding this comment.
x86-64 uses TLS to store the stack canary so it's not needed there.
There was a problem hiding this comment.
Doesn't that mean then that we //can't// use libc/src/compiler/linux/__stack_chk_guard.cpp for x86?
I guess I have a question around do we need to match existing ABI for different architectures as glibc, or can we do our own ABI? If we do our own ABI, I'd much rather have per thread stack canaries, than a single global for all threads.
There was a problem hiding this comment.
I think the baremetal impl in this PR is fine.
For the linux target, we need to do more research because I'm quite sure some architectures use TLS for the canary.
There was a problem hiding this comment.
We already implement support for setting the TLS canary on x86-64 Linux here
llvm-project/libc/startup/linux/x86_64/tls.cpp
Lines 67 to 76 in ebd4dc4
The location of the stack canary is defined by psABI and if we want to change I think it'd be better to propose a change to psABI for the particular architecture.
There was a problem hiding this comment.
There was a problem hiding this comment.
I might be misunderstanding, but take this example: https://godbolt.org/z/84s39bcsY
%fs:40 is the thread local slot for the stack canary? i.e. __stack_chk_guard ?
There was a problem hiding this comment.
Yes, it is. canary can be either accessed via __stack_chk_guard or TLS canary, depending on platform conventions and compiler options. In either case, it is the same value for different threads. Therefore, __stack_chk_guard does not need to be __thread. My guess is that TLS is used to make it harder to override the canary.
There was a problem hiding this comment.
Inside linux kernel, I think there are per-cpu or even per-task canary. But in userspace, at least for MUSL, I only found a single-valued canary.
There was a problem hiding this comment.
FYI, TLS and global variable are two styles of protector guards. This is made explicit in RISC-V cmdopts (https://gcc.gnu.org/onlinedocs/gcc/RISC-V-Options.html):
-mstack-protector-guard=guard
-mstack-protector-guard-reg=reg
-mstack-protector-guard-offset=offsetGenerate stack protection code using canary at guard. Supported locations are ‘global’ for a global canary or ‘tls’ for per-thread canary in the TLS block.
With the latter choice the options -mstack-protector-guard-reg=reg and -mstack-protector-guard-offset=offset furthermore specify which register to use as base register for reading the canary, and from what offset from that base register. There is no default register or offset as this is entirely for use within the Linux kernel.
Ah, GCC does mention per-thread canary, so there is possibility to use different canary for different threads. It is just that some libc implementations decide not to do that by default.
| @@ -19,6 +19,7 @@ set(TARGET_LIBC_ENTRYPOINTS | |||
|
|
|||
| # compiler entrypoints (no corresponding header) | |||
| libc.src.compiler.__stack_chk_fail | |||
| libc.src.compiler.__stack_chk_guard | |||
There was a problem hiding this comment.
Doesn't that mean then that we //can't// use libc/src/compiler/linux/__stack_chk_guard.cpp for x86?
I guess I have a question around do we need to match existing ABI for different architectures as glibc, or can we do our own ABI? If we do our own ABI, I'd much rather have per thread stack canaries, than a single global for all threads.
There was a problem hiding this comment.
I hope to put a request changes for further discussions.
Some concerns from my side:
- Itanium ABI says:
Initialization entries with the same priority from different files (or from other sources such as link command options) will be executed in an unspecified order.
- Does this mean that we do not know when the global guard will be ready? And this is only for C++, only for one ABI.
- Since we do not have control of how static initialization is really implemented in different ABI, will this potentially mess up something? For example, (although unlikely) the
_Global_XXXXfunction of this inserts a stack check, then the ctor alters the value of the global guard which makes_Global_XXXXfails in its epilogue. - My general concern is that
glibc/muslactually initializes stack protection in the early few steps inside_start.libcitself can be compiled with-fstack-protector-XYZ. Giving out explicit control of initialization seems a little bit dangerous to me.
- Calling
getauxvalmay not be desired sincegetauxvaldepends on several functions, those who themselves may want to read the global guard when our libc is compiled with stack protectors enabled. Consider the following approach instead:
#include <config/linux/app.h>
/* .... */
if (&app != nullptr) {
for (auto * p = app.auxv_ptr; *p != AT_NULL; ++p) {
if (*p == AT_RANDOM) { /* initialize */ }
}
}- When pointer width is
$\ge 8$ , MUSL sacrifices a byte (the second byte) to improve the resistance to string functions by setting it to zero. Should we also consider doing it? - This approach diverges the values of TLS canary and global guard. We may need to consider updating the setup of the TLS canary.
I see your point. There's a function attribute we can add to disable stack checks, which is helpful in routines that are responsible for initializing the stack canary in the first place (otherwise the initial value placed on the stack is undefined, which is very likely to result in a call to
Sure, could be a well-named help function.
What platform is that? I'm going to go with "let's worry about that when we need to add support for such platform."
If we have TLS canary, then we don't need a single global, right? |
Sorry for not making it clear. It should also include 64-bit platforms.
Ideally yes. Although it should be rare, is it possible that some platforms' client applications/libs can be either compiled with a global guard or TLS guards? (such that when these objects are linked against libc together, the libc needs to initialize both guards). Maybe we don't need to worry about this yet. |
oh, sorry, I misread your post and missed the Oh, that's a neat idea.
I don't think so. I expect the platform ABI defines one or the other, with no need to support both (for one configuration of the libc).
I think the platform specific startup files need to be aware of this requirement from the rest of the library that the app startup needs to initialize these canaries early. |
This is needed on systems that don't use TLS to store the stack canary.