Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP [libc++][hardening] Overhaul the termination mechanism for hardening #77823

Closed

Conversation

var-const
Copy link
Member

This patch:

  1. Changes how the program is terminated when a hardening assertion is
    triggered (from aborting to trapping).
  2. Introduces a new mechanism for overriding the default (this can be
    done via CMake configuration or by defining a macro; link-time
    overriding is no longer supported).

When hitting a hardening assertion in production, we want to stop the
program as soon as possible. From a security perspective, trapping is
preferable to calling std::abort, so use __builtin_trap in this case
(with the intention of switching to __builtin_verbose_trap once that
becomes available, see https://discourse.llvm.org/t/rfc-adding-builtin-verbose-trap-string-literal/75845).
This also improves the code size because we no longer need to do
a function call to __verbose_abort that in the general case cannot be
inlined.

In the debug hardening mode, the security aspect is less important since
it's not intended to be run in production. For that reason, keep using
__verbose_abort in debug mode.

This patch:
1. Changes how the program is terminated when a hardening assertion is
   triggered (from aborting to trapping).
2. Introduces a new mechanism for overriding the default (this can be
   done via CMake configuration or by defining a macro; link-time
   overriding is no longer supported).

When hitting a hardening assertion in production, we want to stop the
program as soon as possible. From a security perspective, trapping is
preferable to calling `std::abort`, so use `__builtin_trap` in this case
(with the intention of switching to `__builtin_verbose_trap` once that
becomes available, see https://discourse.llvm.org/t/rfc-adding-builtin-verbose-trap-string-literal/75845).
This also improves the code size because we no longer need to do
a function call to `__verbose_abort` that in the general case cannot be
inlined.

In the debug hardening mode, the security aspect is less important since
it's not intended to be run in production. For that reason, keep using
`__verbose_abort` in debug mode.
@var-const var-const requested a review from a team as a code owner January 11, 2024 20:06
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 11, 2024
@var-const var-const added the hardening Issues related to the hardening effort label Jan 11, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 11, 2024

@llvm/pr-subscribers-libcxx

Author: Konstantin Varlamov (var-const)

Changes

This patch:

  1. Changes how the program is terminated when a hardening assertion is
    triggered (from aborting to trapping).
  2. Introduces a new mechanism for overriding the default (this can be
    done via CMake configuration or by defining a macro; link-time
    overriding is no longer supported).

When hitting a hardening assertion in production, we want to stop the
program as soon as possible. From a security perspective, trapping is
preferable to calling std::abort, so use __builtin_trap in this case
(with the intention of switching to __builtin_verbose_trap once that
becomes available, see https://discourse.llvm.org/t/rfc-adding-builtin-verbose-trap-string-literal/75845).
This also improves the code size because we no longer need to do
a function call to __verbose_abort that in the general case cannot be
inlined.

In the debug hardening mode, the security aspect is less important since
it's not intended to be run in production. For that reason, keep using
__verbose_abort in debug mode.


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

6 Files Affected:

  • (modified) libcxx/include/CMakeLists.txt (+1)
  • (modified) libcxx/include/__assert (+24-1)
  • (modified) libcxx/include/__config_site.in (+1)
  • (added) libcxx/include/__verbose_trap (+23)
  • (modified) libcxx/include/module.modulemap.in (+4)
  • (modified) libcxx/utils/generate_iwyu_mapping.py (+2)
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 0fe3ab44d2466e..fa5140dea65842 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -865,6 +865,7 @@ set(files
   __utility/unreachable.h
   __variant/monostate.h
   __verbose_abort
+  __verbose_trap
   algorithm
   any
   array
diff --git a/libcxx/include/__assert b/libcxx/include/__assert
index d4af7e6c7192ab..3885c0ac0fb512 100644
--- a/libcxx/include/__assert
+++ b/libcxx/include/__assert
@@ -12,17 +12,40 @@
 
 #include <__config>
 #include <__verbose_abort>
+#include <__verbose_trap>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
+// _LIBCPP_ASSERT_IMPL
+
+#if defined(_LIBCPP_VERBOSE_TRAP_IMPL_OVERRIDE)
+
+#define _LIBCPP_ASSERT_IMPL(error_message, ...) _LIBCPP_VERBOSE_TRAP_IMPL_OVERRIDE(__VA_ARGS__)
+
+#elif _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
+
+#define _LIBCPP_ASSERT_IMPL(error_message, ...) ((void)error_message, _LIBCPP_VERBOSE_ABORT(__VA_ARGS__))
+
+#else
+
+void __use(const char*, ...);
+#define _LIBCPP_ASSERT_IMPL(error_message, ...) (decltype(__use(__VA_ARGS__))(), _LIBCPP_VERBOSE_TRAP(error_message))
+
+#endif
+
+// _LIBCPP_ASSERT
+
 #define _LIBCPP_ASSERT(expression, message)                                                                            \
   (__builtin_expect(static_cast<bool>(expression), 1)                                                                  \
        ? (void)0                                                                                                       \
-       : _LIBCPP_VERBOSE_ABORT(                                                                                        \
+       : _LIBCPP_ASSERT_IMPL(                                                                                          \
+             message,                                                                                                  \
              "%s:%d: assertion %s failed: %s\n", __builtin_FILE(), __builtin_LINE(), #expression, message))
 
+// _LIBCPP_ASSUME
+
 // TODO: __builtin_assume can currently inhibit optimizations. Until this has been fixed and we can add
 //       assumptions without a clear optimization intent, disable that to avoid worsening the code generation.
 //       See https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609 for a discussion.
diff --git a/libcxx/include/__config_site.in b/libcxx/include/__config_site.in
index 7c002c5bfcf8e7..1227b232dab9af 100644
--- a/libcxx/include/__config_site.in
+++ b/libcxx/include/__config_site.in
@@ -38,6 +38,7 @@
 
 // Hardening.
 #cmakedefine _LIBCPP_HARDENING_MODE_DEFAULT @_LIBCPP_HARDENING_MODE_DEFAULT@
+#cmakedefine _LIBCPP_VERBOSE_TRAP_IMPL_OVERRIDE(...) @_LIBCPP_VERBOSE_TRAP_IMPL_OVERRIDE@
 
 // __USE_MINGW_ANSI_STDIO gets redefined on MinGW
 #ifdef __clang__
diff --git a/libcxx/include/__verbose_trap b/libcxx/include/__verbose_trap
new file mode 100644
index 00000000000000..dfb491d712cc21
--- /dev/null
+++ b/libcxx/include/__verbose_trap
@@ -0,0 +1,23 @@
+// -*- 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 _LIBCPP___VERBOSE_TRAP
+#define _LIBCPP___VERBOSE_TRAP
+
+#include <__availability>
+#include <__config>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+// TODO: use `__builtin_verbose_trap(message) once available
+#define _LIBCPP_VERBOSE_TRAP(message) ((void)message, __builtin_trap())
+
+#endif // _LIBCPP___VERBOSE_TRAP
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index d10670d4faaffc..02f8e4ea20a740 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -630,6 +630,10 @@ module std_private_verbose_abort     [system] {
   header "__verbose_abort"
   export *
 }
+module std_private_verbose_trap      [system] {
+  header "__verbose_trap"
+  export *
+}
 
 module std_private_algorithm_adjacent_find                               [system] { header "__algorithm/adjacent_find.h" }
 module std_private_algorithm_all_of                                      [system] { header "__algorithm/all_of.h" }
diff --git a/libcxx/utils/generate_iwyu_mapping.py b/libcxx/utils/generate_iwyu_mapping.py
index 343538a6cae481..698e434c2ffeec 100644
--- a/libcxx/utils/generate_iwyu_mapping.py
+++ b/libcxx/utils/generate_iwyu_mapping.py
@@ -89,6 +89,8 @@ def generate_map(include):
             continue
         elif i == "__verbose_abort":
             continue
+        elif i == "__verbose_trap":
+            continue
         else:
             panic(i)
 

@var-const var-const marked this pull request as draft January 11, 2024 20:07
@@ -0,0 +1,23 @@
// -*- C++ -*-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our live review just now, we explored the following alternatives (using chrome as an example since we know they override verbose abort right now):

Option #1
=======================
$ cmake ... -DLIBCXX_VERBOSE_TRAP_OVERRIDE="void __chromium_trap(char const*);\n#define _LIBCPP_VERBOSE_TRAP(message) __chromium_trap(message)"

// __verbose_trap.in
#ifndef _LIBCPP___VERBOSE_TRAP
#define _LIBCPP___VERBOSE_TRAP

#include <__availability>
#include <__config>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#  pragma GCC system_header
#endif

#if @LIBCXX_VERBOSE_TRAP_OVERRIDE@
    @LIBCXX_VERBOSE_TRAP_OVERRIDE@
#elif HARDENING_MODE == DEBUG
#   define _LIBCPP_VERBOSE_TRAP(message) ::verbose_abort(...)
#else
#   define _LIBCPP_VERBOSE_TRAP(message) ((void)message, __builtin_trap())
#endif

#endif // _LIBCPP___VERBOSE_TRAP



Option #2
=======================
$ cmake ... -DLIBCXX_VERBOSE_TRAP_OVERRIDE=libcxx/vendor/chrome/verbose_trap.in

// __verbose_trap.in
#ifndef _LIBCPP___VERBOSE_TRAP
#define _LIBCPP___VERBOSE_TRAP

#include <__availability>
#include <__config>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#  pragma GCC system_header
#endif

#if @LIBCXX_VERBOSE_TRAP_OVERRIDE@
    copy-paste the contents of @LIBCXX_VERBOSE_TRAP_OVERRIDE@ at CMake configuration time
#elif HARDENING_MODE == DEBUG
#   define _LIBCPP_VERBOSE_TRAP(message) ::verbose_abort(...)
#else
#   define _LIBCPP_VERBOSE_TRAP(message) ((void)message, __builtin_trap())
#endif

#endif // _LIBCPP___VERBOSE_TRAP


// in libcxx/vendor/chrome/verbose_trap.in
#define _LIBCPP_VERBOSE_TRAP(message) ::whatever()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further discussion: Since we may want to have use cases where the "verbose trap handler" doesn't actually abort the program (effectively deciding to ignore library-level UB), it would probably make sense to rename this. Suggestions:

  • _LIBCPP_LIBRARY_UB_HANDLER(message)
  • _LIBCPP_UB_HANDLER(message)
  • _LIBCPP_ASSERTION_HANDLER(message)
  • _LIBCPP_ASSERTION_FAILURE(message)

There's probably more. The point is that if we use one of these names, using an implementation where don't actually terminate the program is now more natural.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More discussion!

We could actually always use the overriding mechanism but provide our default definition for the assertion handler. That way, the code path for customizing the handler would always be tested, by definition. Something like:

// in CMakeLists.txt
set(LIBCXX_ASSERTION_HANDLER "libcxx/vendor/llvm/assertion_handler.in" CACHE STR)

// in libcxx/include/__assertion_handler.in
#ifndef _LIBCPP___ASSERTION_HANDLER
#define _LIBCPP___ASSERTION_HANDLER

#include <__availability>
#include <__config>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#  pragma GCC system_header
#endif

copy-paste the contents of @LIBCXX_ASSERTION_HANDLER@ at CMake configuration time

#endif // _LIBCPP___ASSERTION_HANDLER

// in libcxx/vendor/llvm/assertion_handler.in
#if HARDENING_MODE == DEBUG
#   define _LIBCPP_VERBOSE_TRAP(message) ::std::__libcpp_verbose_abort(...)
#else
#   define _LIBCPP_VERBOSE_TRAP(message) ((void)message, __builtin_trap())
#endif


#endif

// _LIBCPP_ASSERT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a bit of documentation here that explains that _LIBCPP_ASSERT(expr, msg) is a no-op if expr is true, and it's library-level UB in case expr is false.

@var-const
Copy link
Member Author

Closing in favor of #77883.

@var-const var-const closed this Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hardening Issues related to the hardening effort libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants