[libc] Simple __stack_chk_guard implementation

[petrhosek]

This is needed on systems that don't use TLS to store the stack canary.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Petr Hosek (petrhosek)

Changes

This 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:

• (modified) libc/config/baremetal/arm/entrypoints.txt (+1)
• (modified) libc/config/baremetal/riscv/entrypoints.txt (+1)
• (modified) libc/config/linux/aarch64/entrypoints.txt (+1)
• (modified) libc/config/linux/arm/entrypoints.txt (+8)
• (modified) libc/src/compiler/CMakeLists.txt (+15-2)
• (added) libc/src/compiler/__stack_chk_guard.h (+20)
• (added) libc/src/compiler/baremetal/CMakeLists.txt (+7)
• (added) libc/src/compiler/baremetal/__stack_chk_guard.cpp (+17)
• (added) libc/src/compiler/linux/CMakeLists.txt (+11)
• (added) libc/src/compiler/linux/__stack_chk_guard.cpp (+39)
• (modified) libc/test/src/compiler/CMakeLists.txt (+1)

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 

[michaelrj-google]

overall LGTM, but might want to wait for the getauxval patch to land

[michaelrj-google]

should this also be enabled for x86_64?

[petrhosek]

x86-64 uses TLS to store the stack canary so it's not needed there.

[nickdesaulniers]

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.

[nickdesaulniers]

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.

[petrhosek]

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

	uintptr_t *stack_guard_addr = reinterpret_cast<uintptr_t *>(end_ptr + 40);
	// Setting the stack guard to a random value.
	// We cannot call the get_random function here as the function sets errno on
	// failure. Since errno is implemented via a thread local variable, we cannot
	// use errno before TLS is setup.
	long stack_guard_retval =
	syscall_impl(SYS_getrandom, reinterpret_cast<long>(stack_guard_addr),
	sizeof(uint64_t), 0);
	if (stack_guard_retval < 0)
	syscall_impl(SYS_exit, 1);

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.

[SchrodingerZhu]

see also https://github.com/bminor/musl/blob/f314e133929b6379eccc632bef32eaebb66a7335/src/thread/pthread_create.c#L328

[nickdesaulniers]

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 ?

[SchrodingerZhu]

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.

[SchrodingerZhu]

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.

[SchrodingerZhu]

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=offset

Generate 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.

[nickdesaulniers]

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.

[SchrodingerZhu]

I hope to put a request changes for further discussions.

Some concerns from my side:

1. 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_XXXX function of this inserts a stack check, then the ctor alters the value of the global guard which makes _Global_XXXX fails in its epilogue.
• My general concern is that glibc/musl actually initializes stack protection in the early few steps inside _start. libc itself can be compiled with -fstack-protector-XYZ. Giving out explicit control of initialization seems a little bit dangerous to me.

2. Calling getauxval may not be desired since getauxval depends 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 */ } } }

3. 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?
4. This approach diverges the values of TLS canary and global guard. We may need to consider updating the setup of the TLS canary.

[nickdesaulniers]

Does this mean that we do not know when the global guard will be ready?

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 __stack_chk_fail). no_stack_protector IIRC it was only added to GCC in gcc-10 though. This generally isn't a problem in the Linux kernel because canary initialization is done very early, then you don't need to worry about it.

Consider the following approach instead

Sure, could be a well-named help function.

When pointer width is > 8

What platform is that? I'm going to go with "let's worry about that when we need to add support for such platform."

This approach diverges the values of TLS canary and global guard.

If we have TLS canary, then we don't need a single global, right?

[SchrodingerZhu]

When pointer width is >= 8

Sorry for not making it clear. It should also include 64-bit platforms.
https://github.com/bminor/musl/blob/f314e133929b6379eccc632bef32eaebb66a7335/src/env/__stack_chk_fail.c#L18

If we have TLS canary, then we don't need a single global, right?

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.

[SchrodingerZhu]

I am okay with the change if initialization order is for now out of concern for this approach.
Please also consider further changes related to the auxv_ptr and the protection against string routines.

[nickdesaulniers]

When pointer width is > 8

oh, sorry, I misread your post and missed the = in >=.

https://github.com/bminor/musl/blob/f314e133929b6379eccc632bef32eaebb66a7335/src/env/__stack_chk_fail.c#L18

Oh, that's a neat idea.

is it possible that some platforms' client applications/libs can be either compiled with a global guard or TLS guards?

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 am okay with the change if initialization order is for now out of concern for this approach.

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.