-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[runtimes][PAC] Harden unwinding when possible #143230
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-libunwind Author: Oliver Hunt (ojhunt) ChangesThis hardens the unwinding logic and datastructures on systems that support pointer authentication. The approach taken to hardening is to harden the schemas of as many high value fields in the myriad structs as possible, and then also explicitly qualify local variables referencing privileged or security critical values. This ABI is exposed to the personality functions, and so updating to conform to that is a mandatory change, but to reduce the risk of oracles, the adoption also hardened the locals and datastructures in compiler-rt and libcxxabi. We're gating these on Patch is 51.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143230.diff 14 Files Affected:
diff --git a/compiler-rt/lib/builtins/gcc_personality_v0.c b/compiler-rt/lib/builtins/gcc_personality_v0.c
index ef63a5fb83472..42b992949d2cc 100644
--- a/compiler-rt/lib/builtins/gcc_personality_v0.c
+++ b/compiler-rt/lib/builtins/gcc_personality_v0.c
@@ -30,6 +30,45 @@ EXCEPTION_DISPOSITION _GCC_specific_handler(PEXCEPTION_RECORD, void *, PCONTEXT,
_Unwind_Personality_Fn);
#endif
+#if __has_include(<ptrauth.h>)
+#include <ptrauth.h>
+#endif
+
+#if defined(__APPLE__) && __has_feature(ptrauth_qualifier)
+#if __has_feature(ptrauth_restricted_intptr_qualifier)
+#define PERSONALITY_PTRAUTH_RESTRICTED_INTPTR(key, addressDiscriminated, \
+ discriminatorString) \
+ __ptrauth_restricted_intptr(key, addressDiscriminated, \
+ ptrauth_string_discriminator(discriminatorString))
+#else
+#define PERSONALITY_PTRAUTH_RESTRICTED_INTPTR(key, addressDiscriminated, \
+ discriminatorString) \
+ __ptrauth(key, addressDiscriminated, \
+ ptrauth_string_discriminator(discriminatorString))
+#endif
+#else
+#define PERSONALITY_PTRAUTH_RESTRICTED_INTPTR(key, addressDiscriminated, \
+ discriminatorString)
+#endif
+
+// Helper wrappers for pointer auth qualifiers because we use a lot of variants
+// Suffixes:
+// * PDC : ptrauth_key_process_dependent_code
+// * RA : ptrauth_key_return_address
+// * FN : ptrauth_key_function_pointer
+#define PERSONALITY_PTRAUTH_RI_FN(__discriminator) \
+ PERSONALITY_PTRAUTH_RESTRICTED_INTPTR(ptrauth_key_function_pointer, \
+ /*__address_discriminated=*/1, \
+ __discriminator)
+#define PERSONALITY_PTRAUTH_RI_PDC(__discriminator) \
+ PERSONALITY_PTRAUTH_RESTRICTED_INTPTR(ptrauth_key_process_dependent_code, \
+ /*__address_discriminated=*/1, \
+ __discriminator)
+#define PERSONALITY_PTRAUTH_RI_RA(__discriminator) \
+ PERSONALITY_PTRAUTH_RESTRICTED_INTPTR(ptrauth_key_return_address, \
+ /*__address_discriminated=*/1, \
+ __discriminator)
+
// Pointer encodings documented at:
// http://refspecs.freestandards.org/LSB_1.3.0/gLSB/gLSB/ehframehdr.html
@@ -205,7 +244,8 @@ COMPILER_RT_ABI _Unwind_Reason_Code __gcc_personality_v0(
return continueUnwind(exceptionObject, context);
uintptr_t pc = (uintptr_t)_Unwind_GetIP(context) - 1;
- uintptr_t funcStart = (uintptr_t)_Unwind_GetRegionStart(context);
+ uintptr_t PERSONALITY_PTRAUTH_RI_FN("__gcc_personality_v0'funcStart")
+ funcStart = (uintptr_t)_Unwind_GetRegionStart(context);
uintptr_t pcOffset = pc - funcStart;
// Parse LSDA header.
@@ -224,11 +264,14 @@ COMPILER_RT_ABI _Unwind_Reason_Code __gcc_personality_v0(
const uint8_t *callSiteTableEnd = callSiteTableStart + callSiteTableLength;
const uint8_t *p = callSiteTableStart;
while (p < callSiteTableEnd) {
- uintptr_t start = readEncodedPointer(&p, callSiteEncoding);
- size_t length = readEncodedPointer(&p, callSiteEncoding);
- size_t landingPad = readEncodedPointer(&p, callSiteEncoding);
+ uintptr_t PERSONALITY_PTRAUTH_RI_PDC("__gcc_personality_v0'start")
+ start = readEncodedPointer(&p, callSiteEncoding);
+ size_t PERSONALITY_PTRAUTH_RI_PDC("__gcc_personality_v0'length")
+ length = readEncodedPointer(&p, callSiteEncoding);
+ size_t PERSONALITY_PTRAUTH_RI_PDC("__gcc_personality_v0'landingPadOffset")
+ landingPadOffset = readEncodedPointer(&p, callSiteEncoding);
readULEB128(&p); // action value not used for C code
- if (landingPad == 0)
+ if (landingPadOffset == 0)
continue; // no landing pad for this entry
if ((start <= pcOffset) && (pcOffset < (start + length))) {
// Found landing pad for the PC.
@@ -238,7 +281,21 @@ COMPILER_RT_ABI _Unwind_Reason_Code __gcc_personality_v0(
_Unwind_SetGR(context, __builtin_eh_return_data_regno(0),
(uintptr_t)exceptionObject);
_Unwind_SetGR(context, __builtin_eh_return_data_regno(1), 0);
- _Unwind_SetIP(context, (funcStart + landingPad));
+#define LANDING_PAD_DISCRIMINATOR "__gcc_personality_v0'landingPad"
+ size_t PERSONALITY_PTRAUTH_RI_RA(LANDING_PAD_DISCRIMINATOR)
+ landingPad = funcStart + landingPadOffset;
+#if defined(__APPLE__) && __has_feature(ptrauth_qualifier)
+ uintptr_t stack_pointer = _Unwind_GetGR(context, -2);
+ const uintptr_t existingDiscriminator = ptrauth_blend_discriminator(
+ &landingPad,
+ ptrauth_string_discriminator(LANDING_PAD_DISCRIMINATOR));
+ uintptr_t newIP = (uintptr_t)ptrauth_auth_and_resign(
+ *(void **)&landingPad, ptrauth_key_function_pointer,
+ existingDiscriminator, ptrauth_key_return_address, stack_pointer);
+ _Unwind_SetIP(context, newIP);
+#else
+ _Unwind_SetIP(context, landingPad);
+#endif
return _URC_INSTALL_CONTEXT;
}
}
diff --git a/compiler-rt/lib/profile/InstrProfilingValue.c b/compiler-rt/lib/profile/InstrProfilingValue.c
index a608d41d39e77..cd6ae3d7a4248 100644
--- a/compiler-rt/lib/profile/InstrProfilingValue.c
+++ b/compiler-rt/lib/profile/InstrProfilingValue.c
@@ -83,7 +83,13 @@ __llvm_profile_iterate_data(const __llvm_profile_data *Data) {
/* This method is only used in value profiler mock testing. */
COMPILER_RT_VISIBILITY void *
__llvm_get_function_addr(const __llvm_profile_data *Data) {
- return Data->FunctionPointer;
+ void *FP = Data->FunctionPointer;
+#if __has_feature(ptrauth_calls)
+ // This is only used for tests where we compare against what happens to be
+ // signed pointers.
+ FP = ptrauth_sign_unauthenticated(FP, VALID_CODE_KEY, 0);
+#endif
+ return FP;
}
/* Allocate an array that holds the pointers to the linked lists of
diff --git a/libcxxabi/include/__cxxabi_config.h b/libcxxabi/include/__cxxabi_config.h
index 759445dac91f9..e67d065fe57f3 100644
--- a/libcxxabi/include/__cxxabi_config.h
+++ b/libcxxabi/include/__cxxabi_config.h
@@ -32,7 +32,8 @@
#endif
#if defined(_WIN32)
- #if defined(_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS) || (defined(__MINGW32__) && !defined(_LIBCXXABI_BUILDING_LIBRARY))
+ #if defined(_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS) || \
+ (defined(__MINGW32__) && !defined(_LIBCXXABI_BUILDING_LIBRARY))
#define _LIBCXXABI_HIDDEN
#define _LIBCXXABI_DATA_VIS
#define _LIBCXXABI_FUNC_VIS
@@ -109,4 +110,49 @@
# define _LIBCXXABI_NOEXCEPT noexcept
#endif
+#if __has_include(<ptrauth.h>)
+#include <ptrauth.h>
+#endif
+
+#if defined(__APPLE__) && __has_feature(ptrauth_qualifier)
+# define _LIBCXXABI_PTRAUTH(__key, __address_discriminated, __discriminator) \
+ __ptrauth(__key,__address_discriminated, \
+ ptrauth_string_discriminator(__discriminator))
+// This work around is required to support divergence in spelling
+// during the ptrauth upstreaming process.
+# if __has_feature(ptrauth_restricted_intptr_qualifier)
+# define _LIBCXXABI_PTRAUTH_RESTRICTED_INTPTR(__key, __address_discriminated, \
+ __discriminator) \
+ __ptrauth_restricted_intptr(__key,__address_discriminated, \
+ ptrauth_string_discriminator(__discriminator))
+# else
+# define _LIBCXXABI_PTRAUTH_RESTRICTED_INTPTR(__key, __address_discriminated, \
+ __discriminator) \
+ __ptrauth(__key,__address_discriminated, \
+ ptrauth_string_discriminator(__discriminator))
+# endif
+#else
+# define _LIBCXXABI_PTRAUTH(__key, __address_discriminated, __discriminator)
+# define _LIBCXXABI_PTRAUTH_RESTRICTED_INTPTR(__key, __address_discriminated, \
+ __discriminator)
+#endif
+
+// Helper wrappers for pointer auth qualifiers because we use a lot of variants
+// Suffixes:
+// * _RI : qualifier is __ptrauth_restricted_intptr
+// * PDD : key is ptrauth_key_process_dependent_data
+// * FN : key is ptrauth_key_function_pointer
+#define _LIBCXXABI_PTRAUTH_PDD(__discriminator) \
+ _LIBCXXABI_PTRAUTH(ptrauth_key_process_dependent_data, \
+ /*__address_discriminated=*/1, \
+ __discriminator)
+#define _LIBCXXABI_PTRAUTH_FN(__discriminator) \
+ _LIBCXXABI_PTRAUTH(ptrauth_key_function_pointer, \
+ /*__address_discriminated=*/1, \
+ __discriminator)
+#define _LIBCXXABI_PTRAUTH_RI_PDD(__discriminator) \
+ _LIBCXXABI_PTRAUTH_RESTRICTED_INTPTR(ptrauth_key_process_dependent_data, \
+ /*__address_discriminated=*/1, \
+ __discriminator)
+
#endif // ____CXXABI_CONFIG_H
diff --git a/libcxxabi/src/cxa_exception.h b/libcxxabi/src/cxa_exception.h
index aba08f2992103..4c69d48048f02 100644
--- a/libcxxabi/src/cxa_exception.h
+++ b/libcxxabi/src/cxa_exception.h
@@ -47,10 +47,10 @@ struct _LIBCXXABI_HIDDEN __cxa_exception {
// In Wasm, a destructor returns its argument
void *(_LIBCXXABI_DTOR_FUNC *exceptionDestructor)(void *);
#else
- void (_LIBCXXABI_DTOR_FUNC *exceptionDestructor)(void *);
+ void(_LIBCXXABI_DTOR_FUNC* _LIBCXXABI_PTRAUTH_FN("__cxa_exception::exceptionDestructor") exceptionDestructor)(void*);
#endif
- std::unexpected_handler unexpectedHandler;
- std::terminate_handler terminateHandler;
+ std::unexpected_handler _LIBCXXABI_PTRAUTH_FN("__cxa_exception::unexpectedHandler") unexpectedHandler;
+ std::terminate_handler _LIBCXXABI_PTRAUTH_FN("__cxa_exception::terminateHandler") terminateHandler;
__cxa_exception *nextException;
@@ -61,10 +61,10 @@ struct _LIBCXXABI_HIDDEN __cxa_exception {
int propagationCount;
#else
int handlerSwitchValue;
- const unsigned char *actionRecord;
- const unsigned char *languageSpecificData;
- void *catchTemp;
- void *adjustedPtr;
+ const unsigned char* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::actionRecord") actionRecord;
+ const unsigned char* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::languageSpecificData") languageSpecificData;
+ void* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::catchTemp") catchTemp;
+ void* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::adjustedPtr") adjustedPtr;
#endif
#if !defined(__LP64__) && !defined(_WIN64) && !defined(_LIBCXXABI_ARM_EHABI)
@@ -79,6 +79,8 @@ struct _LIBCXXABI_HIDDEN __cxa_exception {
// http://sourcery.mentor.com/archives/cxx-abi-dev/msg01924.html
// The layout of this structure MUST match the layout of __cxa_exception, with
// primaryException instead of referenceCount.
+// The tags used in the pointer authentication qualifiers also need to match
+// those of the corresponding members in __cxa_exception.
struct _LIBCXXABI_HIDDEN __cxa_dependent_exception {
#if defined(__LP64__) || defined(_WIN64) || defined(_LIBCXXABI_ARM_EHABI)
void* reserve; // padding.
@@ -86,9 +88,9 @@ struct _LIBCXXABI_HIDDEN __cxa_dependent_exception {
#endif
std::type_info *exceptionType;
- void (_LIBCXXABI_DTOR_FUNC *exceptionDestructor)(void *);
- std::unexpected_handler unexpectedHandler;
- std::terminate_handler terminateHandler;
+ void(_LIBCXXABI_DTOR_FUNC* _LIBCXXABI_PTRAUTH_FN("__cxa_exception::exceptionDestructor") exceptionDestructor)(void*);
+ std::unexpected_handler _LIBCXXABI_PTRAUTH_FN("__cxa_exception::unexpectedHandler") unexpectedHandler;
+ std::terminate_handler _LIBCXXABI_PTRAUTH_FN("__cxa_exception::terminateHandler") terminateHandler;
__cxa_exception *nextException;
@@ -99,10 +101,10 @@ struct _LIBCXXABI_HIDDEN __cxa_dependent_exception {
int propagationCount;
#else
int handlerSwitchValue;
- const unsigned char *actionRecord;
- const unsigned char *languageSpecificData;
- void * catchTemp;
- void *adjustedPtr;
+ const unsigned char* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::actionRecord") actionRecord;
+ const unsigned char* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::languageSpecificData") languageSpecificData;
+ void* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::catchTemp") catchTemp;
+ void* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::adjustedPtr") adjustedPtr;
#endif
#if !defined(__LP64__) && !defined(_WIN64) && !defined(_LIBCXXABI_ARM_EHABI)
diff --git a/libcxxabi/src/cxa_personality.cpp b/libcxxabi/src/cxa_personality.cpp
index 5f6e75c5be19c..cbb3f46e0f55c 100644
--- a/libcxxabi/src/cxa_personality.cpp
+++ b/libcxxabi/src/cxa_personality.cpp
@@ -22,6 +22,12 @@
#include "private_typeinfo.h"
#include "unwind.h"
+#if __has_include(<ptrauth.h>)
+#include <ptrauth.h>
+#endif
+
+#include "libunwind.h"
+
// TODO: This is a temporary workaround for libc++abi to recognize that it's being
// built against LLVM's libunwind. LLVM's libunwind started reporting _LIBUNWIND_VERSION
// in LLVM 15 -- we can remove this workaround after shipping LLVM 17. Once we remove
@@ -527,12 +533,19 @@ get_thrown_object_ptr(_Unwind_Exception* unwind_exception)
namespace
{
+#define _LIBCXXABI_PTRAUTH_KEY ptrauth_key_process_dependent_code
+typedef const uint8_t* _LIBCXXABI_PTRAUTH_PDD("scan_results::languageSpecificData") lsd_ptr_t;
+typedef const uint8_t* _LIBCXXABI_PTRAUTH_PDD("scan_results::actionRecord") action_ptr_t;
+#define _LIBCXXABI_PTRAUTH_SCANRESULT_LANDINGPAD_DISC "scan_results::landingPad"
+typedef uintptr_t _LIBCXXABI_PTRAUTH_RI_PDD(_LIBCXXABI_PTRAUTH_SCANRESULT_LANDINGPAD_DISC) landing_pad_t;
+typedef void* _LIBCXXABI_PTRAUTH_PDD(_LIBCXXABI_PTRAUTH_SCANRESULT_LANDINGPAD_DISC) landing_pad_ptr_t;
+
struct scan_results
{
int64_t ttypeIndex; // > 0 catch handler, < 0 exception spec handler, == 0 a cleanup
- const uint8_t* actionRecord; // Currently unused. Retained to ease future maintenance.
- const uint8_t* languageSpecificData; // Needed only for __cxa_call_unexpected
- uintptr_t landingPad; // null -> nothing found, else something found
+ action_ptr_t actionRecord; // Currently unused. Retained to ease future maintenance.
+ lsd_ptr_t languageSpecificData; // Needed only for __cxa_call_unexpected
+ landing_pad_t landingPad; // null -> nothing found, else something found
void* adjustedPtr; // Used in cxa_exception.cpp
_Unwind_Reason_Code reason; // One of _URC_FATAL_PHASE1_ERROR,
// _URC_FATAL_PHASE2_ERROR,
@@ -541,7 +554,33 @@ struct scan_results
};
} // unnamed namespace
+}
+namespace {
+// The logical model for casting authenticated function pointers makes
+// it impossible to directly cast them without breaking the authentication,
+// as a result we need this pair of helpers.
+template <typename PtrType>
+void set_landing_pad_as_ptr(scan_results& results, const PtrType& out) {
+ union {
+ landing_pad_t* as_landing_pad;
+ landing_pad_ptr_t* as_pointer;
+ } u;
+ u.as_landing_pad = &results.landingPad;
+ *u.as_pointer = out;
+}
+
+static const landing_pad_ptr_t& get_landing_pad_as_ptr(const scan_results& results) {
+ union {
+ const landing_pad_t* as_landing_pad;
+ const landing_pad_ptr_t* as_pointer;
+ } u;
+ u.as_landing_pad = &results.landingPad;
+ return *u.as_pointer;
+}
+} // unnamed namespace
+
+extern "C" {
static
void
set_registers(_Unwind_Exception* unwind_exception, _Unwind_Context* context,
@@ -557,7 +596,22 @@ set_registers(_Unwind_Exception* unwind_exception, _Unwind_Context* context,
reinterpret_cast<uintptr_t>(unwind_exception));
_Unwind_SetGR(context, __builtin_eh_return_data_regno(1),
static_cast<uintptr_t>(results.ttypeIndex));
+#if defined(__APPLE__) && __has_feature(ptrauth_qualifier)
+ auto stack_pointer = _Unwind_GetGR(context, UNW_REG_SP);
+ // We manually re-sign the IP as the __ptrauth qualifiers cannot
+ // express the required relationship with the destination address
+ const auto existingDiscriminator = ptrauth_blend_discriminator(
+ &results.landingPad,
+ ptrauth_string_discriminator(_LIBCXXABI_PTRAUTH_SCANRESULT_LANDINGPAD_DISC));
+ unw_word_t newIP = (unw_word_t)ptrauth_auth_and_resign(*(void**)&results.landingPad,
+ _LIBCXXABI_PTRAUTH_KEY,
+ existingDiscriminator,
+ ptrauth_key_return_address,
+ stack_pointer);
+ _Unwind_SetIP(context, newIP);
+#else
_Unwind_SetIP(context, results.landingPad);
+#endif
}
/*
@@ -691,12 +745,12 @@ static void scan_eh_tab(scan_results &results, _Unwind_Action actions,
// The call sites are ordered in increasing value of start
uintptr_t start = readEncodedPointer(&callSitePtr, callSiteEncoding);
uintptr_t length = readEncodedPointer(&callSitePtr, callSiteEncoding);
- uintptr_t landingPad = readEncodedPointer(&callSitePtr, callSiteEncoding);
+ landing_pad_t landingPad = readEncodedPointer(&callSitePtr, callSiteEncoding);
uintptr_t actionEntry = readULEB128(&callSitePtr);
if ((start <= ipOffset) && (ipOffset < (start + length)))
#else // __USING_SJLJ_EXCEPTIONS__ || __WASM_EXCEPTIONS__
// ip is 1-based index into this table
- uintptr_t landingPad = readULEB128(&callSitePtr);
+ landing_pad_t landingPad = readULEB128(&callSitePtr);
uintptr_t actionEntry = readULEB128(&callSitePtr);
if (--ip == 0)
#endif // __USING_SJLJ_EXCEPTIONS__ || __WASM_EXCEPTIONS__
@@ -935,8 +989,7 @@ __gxx_personality_v0
results.ttypeIndex = exception_header->handlerSwitchValue;
results.actionRecord = exception_header->actionRecord;
results.languageSpecificData = exception_header->languageSpecificData;
- results.landingPad =
- reinterpret_cast<uintptr_t>(exception_header->catchTemp);
+ set_landing_pad_as_ptr(results, exception_header->catchTemp);
results.adjustedPtr = exception_header->adjustedPtr;
// Jump to the handler.
@@ -970,7 +1023,7 @@ __gxx_personality_v0
exc->handlerSwitchValue = static_cast<int>(results.ttypeIndex);
exc->actionRecord = results.actionRecord;
exc->languageSpecificData = results.languageSpecificData;
- exc->catchTemp = reinterpret_cast<void*>(results.landingPad);
+ exc->catchTemp = get_landing_pad_as_ptr(results);
exc->adjustedPtr = results.adjustedPtr;
#ifdef __WASM_EXCEPTIONS__
// Wasm only uses a single phase (_UA_SEARCH_PHASE), so save the
diff --git a/libunwind/include/libunwind.h b/libunwind/include/libunwind.h
index b2dae8feed9a3..e7375bbca1b3d 100644
--- a/libunwind/include/libunwind.h
+++ b/libunwind/include/libunwind.h
@@ -43,6 +43,61 @@
#define LIBUNWIND_AVAIL
#endif
+#if __has_include(<ptrauth.h>)
+#include <ptrauth.h>
+#endif
+
+#if defined(__APPLE__) && __has_feature(ptrauth_qualifier)
+#define _LIBUNWIND_PTRAUTH(__key, __address_discriminated, __discriminator) \
+ __ptrauth(__key, __address_discriminated, \
+ ptrauth_string_discriminator(__discriminator))
+// This work around is required to support divergence in spelling
+// developed during the ptrauth upstreaming process.
+#if __has_feature(ptrauth_restricted_intptr_qualifier)
+#define _LIBUNWIND_PTRAUTH_RESTRICTED_INTPTR(__key, __address_discriminated, \
+ __discriminator) \
+ __ptrauth_restricted_intptr(__key, __address_discriminated, \
+ ptrauth_string_discriminator(__discriminator...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,cpp,c,hpp -- compiler-rt/lib/builtins/crtbegin.c compiler-rt/lib/builtins/gcc_personality_v0.c compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h compiler-rt/test/asan/TestCases/Darwin/linked-only.cpp compiler-rt/test/asan/TestCases/zero_page_pc.cpp compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp compiler-rt/test/ubsan/TestCases/TypeCheck/vptr-corrupted-vtable-itanium.cpp libcxx/src/include/overridable_function.h libcxxabi/include/__cxxabi_config.h libcxxabi/src/cxa_exception.h libcxxabi/src/cxa_personality.cpp libcxxabi/src/private_typeinfo.cpp libunwind/include/libunwind.h libunwind/src/AddressSpace.hpp libunwind/src/CompactUnwinder.hpp libunwind/src/DwarfInstructions.hpp libunwind/src/DwarfParser.hpp libunwind/src/Registers.hpp libunwind/src/UnwindCursor.hpp libunwind/src/UnwindLevel1.c libunwind/src/libunwind.cpp
View the diff from clang-format here.diff --git a/compiler-rt/lib/builtins/crtbegin.c b/compiler-rt/lib/builtins/crtbegin.c
index b743ee472..cb8694f0c 100644
--- a/compiler-rt/lib/builtins/crtbegin.c
+++ b/compiler-rt/lib/builtins/crtbegin.c
@@ -72,7 +72,7 @@ __attribute__((section(".init_array"), used)) static void *__init =
ptrauth_sign_constant(&__do_init, ptrauth_key_init_fini_pointer,
__ptrauth_init_fini_discriminator);
# endif
-# elif __crtbegin_has_ptrauth
+#elif __crtbegin_has_ptrauth
# ifdef __aarch64__
// If ptrauth_init_fini feature is not present, compiler emits raw unsigned
// pointers in .init_array. Use inline assembly to avoid implicit signing of
@@ -154,7 +154,7 @@ __attribute__((section(".fini_array"), used)) static void *__fini =
ptrauth_sign_constant(&__do_fini, ptrauth_key_init_fini_pointer,
__ptrauth_init_fini_discriminator);
# endif
-# elif __crtbegin_has_ptrauth
+#elif __crtbegin_has_ptrauth
# ifdef __aarch64__
// If ptrauth_init_fini feature is not present, compiler emits raw unsigned
// pointers in .fini_array. Use inline assembly to avoid implicit signing of
diff --git a/compiler-rt/lib/builtins/gcc_personality_v0.c b/compiler-rt/lib/builtins/gcc_personality_v0.c
index b7e2d5ba9..74c567244 100644
--- a/compiler-rt/lib/builtins/gcc_personality_v0.c
+++ b/compiler-rt/lib/builtins/gcc_personality_v0.c
@@ -37,11 +37,11 @@ EXCEPTION_DISPOSITION _GCC_specific_handler(PEXCEPTION_RECORD, void *, PCONTEXT,
#if __has_feature(ptrauth_restricted_intptr_qualifier)
#define __ptrauth_gcc_personality_intptr(key, addressDiscriminated, \
- discriminator) \
+ discriminator) \
__ptrauth_restricted_intptr(key, addressDiscriminated, discriminator)
#else
#define __ptrauth_gcc_personality_intptr(key, addressDiscriminated, \
- discriminator) \
+ discriminator) \
__ptrauth(key, addressDiscriminated, discriminator)
#endif
#else
@@ -52,25 +52,30 @@ EXCEPTION_DISPOSITION _GCC_specific_handler(PEXCEPTION_RECORD, void *, PCONTEXT,
#define __ptrauth_gcc_personality_func_key ptrauth_key_function_pointer
// ptrauth_string_discriminator("__gcc_personality_v0'funcStart") == 0xDFEB
-#define __ptrauth_gcc_personality_func_start \
- __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, 0xDFEB)
+#define __ptrauth_gcc_personality_func_start \
+ __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, \
+ 0xDFEB)
// ptrauth_string_discriminator("__gcc_personality_v0'start") == 0x52DC
-#define __ptrauth_gcc_personality_start \
- __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, 0x52DC)
+#define __ptrauth_gcc_personality_start \
+ __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, \
+ 0x52DC)
// ptrauth_string_discriminator("__gcc_personality_v0'length") == 0xFFF7
-#define __ptrauth_gcc_personality_length \
- __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, 0xFFF7)
+#define __ptrauth_gcc_personality_length \
+ __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, \
+ 0xFFF7)
-// ptrauth_string_discriminator("__gcc_personality_v0'landingPadOffset") == 0x6498
-#define __ptrauth_gcc_personality_lpoffset \
- __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, 0x6498)
+// ptrauth_string_discriminator("__gcc_personality_v0'landingPadOffset") ==
+// 0x6498
+#define __ptrauth_gcc_personality_lpoffset \
+ __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, \
+ 0x6498)
// ptrauth_string_discriminator("__gcc_personality_v0'landingPad") == 0xA134
#define __ptrauth_gcc_personality_lpad_disc 0xA134
-#define __ptrauth_gcc_personality_lpad \
- __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, \
+#define __ptrauth_gcc_personality_lpad \
+ __ptrauth_gcc_personality_intptr(__ptrauth_gcc_personality_func_key, 1, \
__ptrauth_gcc_personality_lpad_disc)
// Pointer encodings documented at:
@@ -249,7 +254,7 @@ COMPILER_RT_ABI _Unwind_Reason_Code __gcc_personality_v0(
uintptr_t pc = (uintptr_t)_Unwind_GetIP(context) - 1;
uintptr_t __ptrauth_gcc_personality_func_start funcStart =
- (uintptr_t)_Unwind_GetRegionStart(context);
+ (uintptr_t)_Unwind_GetRegionStart(context);
uintptr_t pcOffset = pc - funcStart;
// Parse LSDA header.
@@ -269,11 +274,11 @@ COMPILER_RT_ABI _Unwind_Reason_Code __gcc_personality_v0(
const uint8_t *p = callSiteTableStart;
while (p < callSiteTableEnd) {
uintptr_t __ptrauth_gcc_personality_start start =
- readEncodedPointer(&p, callSiteEncoding);
+ readEncodedPointer(&p, callSiteEncoding);
size_t __ptrauth_gcc_personality_length length =
- readEncodedPointer(&p, callSiteEncoding);
+ readEncodedPointer(&p, callSiteEncoding);
size_t __ptrauth_gcc_personality_lpoffset landingPadOffset =
- readEncodedPointer(&p, callSiteEncoding);
+ readEncodedPointer(&p, callSiteEncoding);
readULEB128(&p); // action value not used for C code
if (landingPadOffset == 0)
continue; // no landing pad for this entry
@@ -289,20 +294,16 @@ COMPILER_RT_ABI _Unwind_Reason_Code __gcc_personality_v0(
funcStart + landingPadOffset;
#if __gcc_personality_has_ptrauth
uintptr_t stackPointer = _Unwind_GetGR(context, -2);
- const uintptr_t existingDiscriminator =
- ptrauth_blend_discriminator(&landingPad,
- __ptrauth_gcc_personality_lpad_disc);
+ const uintptr_t existingDiscriminator = ptrauth_blend_discriminator(
+ &landingPad, __ptrauth_gcc_personality_lpad_disc);
// newIP is authenticated as if it were qualified with a pseudo qualifier
// along the lines of:
// __ptrauth(ptrauth_key_return_address, <stackPointer>, 0)
// where the stack pointer is used in place of the strict storage
// address.
- uintptr_t newIP =
- (uintptr_t)ptrauth_auth_and_resign(*(void **)&landingPad,
- __ptrauth_gcc_personality_func_key,
- existingDiscriminator,
- ptrauth_key_return_address,
- stackPointer);
+ uintptr_t newIP = (uintptr_t)ptrauth_auth_and_resign(
+ *(void **)&landingPad, __ptrauth_gcc_personality_func_key,
+ existingDiscriminator, ptrauth_key_return_address, stackPointer);
_Unwind_SetIP(context, newIP);
#else
_Unwind_SetIP(context, landingPad);
diff --git a/compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp b/compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp
index 1dae589cd..20e8db7ee 100644
--- a/compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp
+++ b/compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp
@@ -9,7 +9,7 @@
#include <cstdint>
#include <cstdio>
#if __has_feature(ptrauth_calls) || defined(__PTRAUTH__)
- #include <ptrauth.h>
+#include <ptrauth.h>
#else
#define ptrauth_strip(__value, __key) (__value)
#endif
diff --git a/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr-corrupted-vtable-itanium.cpp b/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr-corrupted-vtable-itanium.cpp
index ccee5eea3..5d216ff25 100644
--- a/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr-corrupted-vtable-itanium.cpp
+++ b/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr-corrupted-vtable-itanium.cpp
@@ -7,10 +7,10 @@
#include <typeinfo>
#if __has_feature(ptrauth_calls) || defined(__PTRAUTH__)
-#define __has_ptrauth 1
-#include <ptrauth.h>
+# define __has_ptrauth 1
+# include <ptrauth.h>
#else
-#define __has_ptrauth 0
+# define __has_ptrauth 0
#endif
struct S {
diff --git a/libcxxabi/include/__cxxabi_config.h b/libcxxabi/include/__cxxabi_config.h
index d49c291ca..297f38915 100644
--- a/libcxxabi/include/__cxxabi_config.h
+++ b/libcxxabi/include/__cxxabi_config.h
@@ -112,32 +112,25 @@
# define __ptrauth_cxxabi_has_ptrauth 1
// ptrauth_string_discriminator("__cxa_exception::actionRecord") == 0xFC91
-# define __ptrauth_cxxabi_action_record \
- __ptrauth(ptrauth_key_process_dependent_data, 1, 0xFC91)
+# define __ptrauth_cxxabi_action_record __ptrauth(ptrauth_key_process_dependent_data, 1, 0xFC91)
// ptrauth_string_discriminator("__cxa_exception::languageSpecificData") == 0xE8EE
-# define __ptrauth_cxxabi_lsd \
- __ptrauth(ptrauth_key_process_dependent_data, 1, 0xE8EE)
+# define __ptrauth_cxxabi_lsd __ptrauth(ptrauth_key_process_dependent_data, 1, 0xE8EE)
// ptrauth_string_discriminator("__cxa_exception::catchTemp") == 0xFA58
-# define __ptrauth_cxxabi_catch_temp \
- __ptrauth(ptrauth_key_process_dependent_data, 1, 0xFA58)
+# define __ptrauth_cxxabi_catch_temp __ptrauth(ptrauth_key_process_dependent_data, 1, 0xFA58)
// ptrauth_string_discriminator("__cxa_exception::adjustedPtr") == 0x99E4
-# define __ptrauth_cxxabi_adjusted_ptr \
- __ptrauth(ptrauth_key_process_dependent_data, 1, 0x99E4)
+# define __ptrauth_cxxabi_adjusted_ptr __ptrauth(ptrauth_key_process_dependent_data, 1, 0x99E4)
// ptrauth_string_discriminator("__cxa_exception::unexpectedHandler") == 0x99A9
-# define __ptrauth_cxxabi_unexpected_handler \
- __ptrauth(ptrauth_key_function_pointer, 1, 0x99A9)
+# define __ptrauth_cxxabi_unexpected_handler __ptrauth(ptrauth_key_function_pointer, 1, 0x99A9)
// ptrauth_string_discriminator("__cxa_exception::terminateHandler") == 0x0886)
-# define __ptrauth_cxxabi_terminate_handler \
- __ptrauth(ptrauth_key_function_pointer, 1, 0x886)
+# define __ptrauth_cxxabi_terminate_handler __ptrauth(ptrauth_key_function_pointer, 1, 0x886)
// ptrauth_string_discriminator("__cxa_exception::exceptionDestructor") == 0xC088
-# define __ptrauth_cxxabi_exception_destructor \
- __ptrauth(ptrauth_key_function_pointer, 1, 0xC088)
+# define __ptrauth_cxxabi_exception_destructor __ptrauth(ptrauth_key_function_pointer, 1, 0xC088)
#else
diff --git a/libcxxabi/src/cxa_exception.h b/libcxxabi/src/cxa_exception.h
index 97be422cc..aa8969e59 100644
--- a/libcxxabi/src/cxa_exception.h
+++ b/libcxxabi/src/cxa_exception.h
@@ -47,8 +47,7 @@ struct _LIBCXXABI_HIDDEN __cxa_exception {
// In Wasm, a destructor returns its argument
void *(_LIBCXXABI_DTOR_FUNC *exceptionDestructor)(void *);
#else
- void (_LIBCXXABI_DTOR_FUNC* __ptrauth_cxxabi_exception_destructor
- exceptionDestructor)(void*);
+ void(_LIBCXXABI_DTOR_FUNC* __ptrauth_cxxabi_exception_destructor exceptionDestructor)(void*);
#endif
std::unexpected_handler __ptrauth_cxxabi_unexpected_handler unexpectedHandler;
std::terminate_handler __ptrauth_cxxabi_terminate_handler terminateHandler;
@@ -89,8 +88,7 @@ struct _LIBCXXABI_HIDDEN __cxa_dependent_exception {
#endif
std::type_info *exceptionType;
- void (_LIBCXXABI_DTOR_FUNC* __ptrauth_cxxabi_exception_destructor
- exceptionDestructor)(void*);
+ void(_LIBCXXABI_DTOR_FUNC* __ptrauth_cxxabi_exception_destructor exceptionDestructor)(void*);
std::unexpected_handler __ptrauth_cxxabi_unexpected_handler unexpectedHandler;
std::terminate_handler __ptrauth_cxxabi_terminate_handler terminateHandler;
diff --git a/libcxxabi/src/cxa_personality.cpp b/libcxxabi/src/cxa_personality.cpp
index 1d9e5545d..90a97806e 100644
--- a/libcxxabi/src/cxa_personality.cpp
+++ b/libcxxabi/src/cxa_personality.cpp
@@ -28,38 +28,33 @@
// of maintaining a record of how they were originally produced.
// ptrauth_string_discriminator("scan_results::languageSpecificData") == 0xE50D)
-#define __ptrauth_scan_results_lsd \
- __ptrauth(ptrauth_key_process_dependent_code, 1, 0xE50D)
+# define __ptrauth_scan_results_lsd __ptrauth(ptrauth_key_process_dependent_code, 1, 0xE50D)
// ptrauth_string_discriminator("scan_results::actionRecord") == 0x9823
-#define __ptrauth_scan_results_action_record \
- __ptrauth(ptrauth_key_process_dependent_code, 1, 0x9823)
+# define __ptrauth_scan_results_action_record __ptrauth(ptrauth_key_process_dependent_code, 1, 0x9823)
// scan result is broken up as we have a manual re-sign that requires each component
-#define __ptrauth_scan_results_landingpad_key ptrauth_key_process_dependent_code
+# define __ptrauth_scan_results_landingpad_key ptrauth_key_process_dependent_code
// ptrauth_string_discriminator("scan_results::landingPad") == 0xD27C
-#define __ptrauth_scan_results_landingpad_disc 0xD27C
-#define __ptrauth_scan_results_landingpad \
- __ptrauth(__ptrauth_scan_results_landingpad_key, 1, __ptrauth_scan_results_landingpad_disc)
-
-#if __has_extension(__ptrauth_restricted_intptr)
-#define __ptrauth_scan_results_landingpad_intptr \
- __ptrauth_restricted_intptr(__ptrauth_scan_results_landingpad_key, 1, \
- __ptrauth_scan_results_landingpad_disc)
-#else
-#define __ptrauth_scan_results_landingpad_intptr \
- __ptrauth(__ptrauth_scan_results_landingpad_key, 1, \
- __ptrauth_scan_results_landingpad_disc)
-#endif
+# define __ptrauth_scan_results_landingpad_disc 0xD27C
+# define __ptrauth_scan_results_landingpad \
+ __ptrauth(__ptrauth_scan_results_landingpad_key, 1, __ptrauth_scan_results_landingpad_disc)
+
+# if __has_extension(__ptrauth_restricted_intptr)
+# define __ptrauth_scan_results_landingpad_intptr \
+ __ptrauth_restricted_intptr(__ptrauth_scan_results_landingpad_key, 1, __ptrauth_scan_results_landingpad_disc)
+# else
+# define __ptrauth_scan_results_landingpad_intptr \
+ __ptrauth(__ptrauth_scan_results_landingpad_key, 1, __ptrauth_scan_results_landingpad_disc)
+# endif
#else
-#define __ptrauth_scan_results_lsd
-#define __ptrauth_scan_results_action_record
-#define __ptrauth_scan_results_landingpad
-#define __ptrauth_scan_results_landingpad_intptr
+# define __ptrauth_scan_results_lsd
+# define __ptrauth_scan_results_action_record
+# define __ptrauth_scan_results_landingpad
+# define __ptrauth_scan_results_landingpad_intptr
#endif
-
// TODO: This is a temporary workaround for libc++abi to recognize that it's being
// built against LLVM's libunwind. LLVM's libunwind started reporting _LIBUNWIND_VERSION
// in LLVM 15 -- we can remove this workaround after shipping LLVM 17. Once we remove
@@ -573,9 +568,9 @@ typedef void* __ptrauth_scan_results_landingpad landing_pad_ptr_t;
struct scan_results
{
int64_t ttypeIndex; // > 0 catch handler, < 0 exception spec handler, == 0 a cleanup
- action_ptr_t actionRecord; // Currently unused. Retained to ease future maintenance.
- lsd_ptr_t languageSpecificData; // Needed only for __cxa_call_unexpected
- landing_pad_t landingPad; // null -> nothing found, else something found
+ action_ptr_t actionRecord; // Currently unused. Retained to ease future maintenance.
+ lsd_ptr_t languageSpecificData; // Needed only for __cxa_call_unexpected
+ landing_pad_t landingPad; // null -> nothing found, else something found
void* adjustedPtr; // Used in cxa_exception.cpp
_Unwind_Reason_Code reason; // One of _URC_FATAL_PHASE1_ERROR,
// _URC_FATAL_PHASE2_ERROR,
@@ -630,13 +625,11 @@ set_registers(_Unwind_Exception* unwind_exception, _Unwind_Context* context,
auto stackPointer = _Unwind_GetGR(context, UNW_REG_SP);
// We manually re-sign the IP as the __ptrauth qualifiers cannot
// express the required relationship with the destination address
- const auto existingDiscriminator = ptrauth_blend_discriminator(
- &results.landingPad, __ptrauth_scan_results_landingpad_disc);
+ const auto existingDiscriminator =
+ ptrauth_blend_discriminator(&results.landingPad, __ptrauth_scan_results_landingpad_disc);
unw_word_t newIP /* opaque __ptrauth(ptrauth_key_return_address, stackPointer, 0) */ =
- (unw_word_t)ptrauth_auth_and_resign(*(void**)&results.landingPad,
- __ptrauth_scan_results_landingpad_key,
- existingDiscriminator,
- ptrauth_key_return_address, stackPointer);
+ (unw_word_t)ptrauth_auth_and_resign(*(void**)&results.landingPad, __ptrauth_scan_results_landingpad_key,
+ existingDiscriminator, ptrauth_key_return_address, stackPointer);
_Unwind_SetIP(context, newIP);
#else
_Unwind_SetIP(context, results.landingPad);
diff --git a/libcxxabi/src/private_typeinfo.cpp b/libcxxabi/src/private_typeinfo.cpp
index f3cf1e5be..98aee182e 100644
--- a/libcxxabi/src/private_typeinfo.cpp
+++ b/libcxxabi/src/private_typeinfo.cpp
@@ -54,7 +54,7 @@
#endif
#if __ptrauth_cxxabi_has_ptrauth
-#include <ptrauth.h>
+# include <ptrauth.h>
#endif
template <typename T>
diff --git a/libunwind/include/libunwind.h b/libunwind/include/libunwind.h
index 1160a8a04..6c1d703b0 100644
--- a/libunwind/include/libunwind.h
+++ b/libunwind/include/libunwind.h
@@ -45,104 +45,119 @@
#if __has_feature(ptrauth_calls) || defined(__PTRAUTH__)
- #include <ptrauth.h>
+#include <ptrauth.h>
- // Local "has pointer auth" macro to for backwards compatibility
- // with compilers that do not provide the __PTRAUTH__ macro
- #define __libunwind_has_ptrauth 1
+// Local "has pointer auth" macro to for backwards compatibility
+// with compilers that do not provide the __PTRAUTH__ macro
+#define __libunwind_has_ptrauth 1
- #if __has_extension(ptrauth_restricted_intptr_qualifier)
- #define __unwind_ptrauth_restricted_intptr(...) \
- __ptrauth_restricted_intptr(__VA_ARGS__)
- #else
- #define __unwind_ptrauth_restricted_intptr(...) \
- __ptrauth(__VA_ARGS__)
- #endif
+#if __has_extension(ptrauth_restricted_intptr_qualifier)
+#define __unwind_ptrauth_restricted_intptr(...) \
+ __ptrauth_restricted_intptr(__VA_ARGS__)
+#else
+#define __unwind_ptrauth_restricted_intptr(...) __ptrauth(__VA_ARGS__)
+#endif
// ptrauth_string_discriminator("unw_proc_info_t::handler") == 0x7405
- #define __ptrauth_unwind_upi_handler_disc 0x7405
+#define __ptrauth_unwind_upi_handler_disc 0x7405
- #define __ptrauth_unwind_upi_handler \
- __ptrauth(ptrauth_key_function_pointer, 1, __ptrauth_unwind_upi_handler_disc)
+#define __ptrauth_unwind_upi_handler \
+ __ptrauth(ptrauth_key_function_pointer, 1, __ptrauth_unwind_upi_handler_disc)
- #define __ptrauth_unwind_upi_handler_intptr \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_function_pointer, 1,\
- __ptrauth_unwind_upi_handler_disc)
+#define __ptrauth_unwind_upi_handler_intptr \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_function_pointer, 1, \
+ __ptrauth_unwind_upi_handler_disc)
// ptrauth_string_discriminator("unw_proc_info_t::start_ip") == 0xCA2C
- #define __ptrauth_unwind_upi_startip \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_independent_code, 1, 0xCA2C)
+#define __ptrauth_unwind_upi_startip \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_independent_code, 1, \
+ 0xCA2C)
// ptrauth_string_discriminator("unw_proc_info_t::end_ip") == 0xE183
- #define __ptrauth_unwind_upi_endip \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_independent_code, 1, 0xE183)
+#define __ptrauth_unwind_upi_endip \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_independent_code, 1, \
+ 0xE183)
// ptrauth_string_discriminator("unw_proc_info_t::lsda") == 0x83DE
- #define __ptrauth_unwind_upi_lsda \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, 0x83DE)
+#define __ptrauth_unwind_upi_lsda \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, \
+ 0x83DE)
// ptrauth_string_discriminator("unw_proc_info_t::flags") == 0x79A1
- #define __ptrauth_unwind_upi_flags \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, 0x79A1)
+#define __ptrauth_unwind_upi_flags \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, \
+ 0x79A1)
// ptrauth_string_discriminator("unw_proc_info_t::unwind_info") == 0xC20C
- #define __ptrauth_unwind_upi_info \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, 0xC20C)
+#define __ptrauth_unwind_upi_info \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, \
+ 0xC20C)
// ptrauth_string_discriminator("unw_proc_info_t::extra") == 0x03DF
- #define __ptrauth_unwind_upi_extra \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, 0x03DF)
+#define __ptrauth_unwind_upi_extra \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, \
+ 0x03DF)
// ptrauth_string_discriminator("Registers_arm64::link_reg_t") == 0x8301
- #define __ptrauth_unwind_registers_arm64_link_reg \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_code, 1, 0x8301)
+#define __ptrauth_unwind_registers_arm64_link_reg \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_code, 1, \
+ 0x8301)
// ptrauth_string_discriminator("UnwindInfoSections::dso_base") == 0x4FF5
- #define __ptrauth_unwind_uis_dso_base \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, 0x4FF5)
+#define __ptrauth_unwind_uis_dso_base \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, \
+ 0x4FF5)
// ptrauth_string_discriminator("UnwindInfoSections::dwarf_section") == 0x4974
- #define __ptrauth_unwind_uis_dwarf_section \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, 0x4974)
-
-// ptrauth_string_discriminator("UnwindInfoSections::dwarf_section_length") == 0x2A9A
- #define __ptrauth_unwind_uis_dwarf_section_length \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, 0x2A9A)
-
-// ptrauth_string_discriminator("UnwindInfoSections::compact_unwind_section") == 0xA27B
- #define __ptrauth_unwind_uis_compact_unwind_section \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, 0xA27B)
-
-// ptrauth_string_discriminator("UnwindInfoSections::compact_unwind_section_length") == 0x5D0A
- #define __ptrauth_unwind_uis_compact_unwind_section_length \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, 0x5D0A)
+#define __ptrauth_unwind_uis_dwarf_section \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, \
+ 0x4974)
+
+// ptrauth_string_discriminator("UnwindInfoSections::dwarf_section_length") ==
+// 0x2A9A
+#define __ptrauth_unwind_uis_dwarf_section_length \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, \
+ 0x2A9A)
+
+// ptrauth_string_discriminator("UnwindInfoSections::compact_unwind_section") ==
+// 0xA27B
+#define __ptrauth_unwind_uis_compact_unwind_section \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, \
+ 0xA27B)
+
+// ptrauth_string_discriminator("UnwindInfoSections::compact_unwind_section_length")
+// == 0x5D0A
+#define __ptrauth_unwind_uis_compact_unwind_section_length \
+ __unwind_ptrauth_restricted_intptr(ptrauth_key_process_dependent_data, 1, \
+ 0x5D0A)
// ptrauth_string_discriminator("CIE_Info::personality") == 0x6A40
- #define __ptrauth_unwind_cie_info_personality_disc 0x6A40
- #define __ptrauth_unwind_cie_info_personality \
- __unwind_ptrauth_restricted_intptr(ptrauth_key_function_pointer, 1, \
- __ptrauth_unwind_cie_info_personality_disc)
+#define __ptrauth_unwind_cie_info_personality_disc 0x6A40
+#define __ptrauth_unwind_cie_info_personality \
+ __unwind_ptrauth_restricted_intptr( \
+ ptrauth_key_function_pointer, 1, \
+ __ptrauth_unwind_cie_info_personality_disc)
#else
- // We explicitly define the not enabled case so that checks fail if they
- // do not include the correct definitions.
- #define __libunwind_has_ptrauth 0
-
- #define __ptrauth_unwind_upi_handler
- #define __ptrauth_unwind_upi_handler_intptr
- #define __ptrauth_unwind_upi_startip
- #define __ptrauth_unwind_upi_endip
- #define __ptrauth_unwind_upi_lsda
- #define __ptrauth_unwind_upi_flags
- #define __ptrauth_unwind_upi_info
- #define __ptrauth_unwind_upi_extra
- #define __ptrauth_unwind_registers_arm64_link_reg
- #define __ptrauth_unwind_uis_dso_base
- #define __ptrauth_unwind_uis_dwarf_section
- #define __ptrauth_unwind_uis_dwarf_section_length
- #define __ptrauth_unwind_uis_compact_unwind_section
- #define __ptrauth_unwind_uis_compact_unwind_section_length
- #define __ptrauth_unwind_cie_info_personality
+// We explicitly define the not enabled case so that checks fail if they
+// do not include the correct definitions.
+#define __libunwind_has_ptrauth 0
+
+#define __ptrauth_unwind_upi_handler
+#define __ptrauth_unwind_upi_handler_intptr
+#define __ptrauth_unwind_upi_startip
+#define __ptrauth_unwind_upi_endip
+#define __ptrauth_unwind_upi_lsda
+#define __ptrauth_unwind_upi_flags
+#define __ptrauth_unwind_upi_info
+#define __ptrauth_unwind_upi_extra
+#define __ptrauth_unwind_registers_arm64_link_reg
+#define __ptrauth_unwind_uis_dso_base
+#define __ptrauth_unwind_uis_dwarf_section
+#define __ptrauth_unwind_uis_dwarf_section_length
+#define __ptrauth_unwind_uis_compact_unwind_section
+#define __ptrauth_unwind_uis_compact_unwind_section_length
+#define __ptrauth_unwind_cie_info_personality
#endif
@@ -191,18 +206,23 @@ typedef double unw_fpreg_t;
#endif
struct unw_proc_info_t {
- unw_word_t __ptrauth_unwind_upi_startip start_ip; /* start address of function */
- unw_word_t __ptrauth_unwind_upi_endip end_ip; /* address after end of function */
- unw_word_t __ptrauth_unwind_upi_lsda lsda; /* address of language specific data area, */
- /* or zero if not used */
+ unw_word_t __ptrauth_unwind_upi_startip
+ start_ip; /* start address of function */
+ unw_word_t __ptrauth_unwind_upi_endip
+ end_ip; /* address after end of function */
+ unw_word_t __ptrauth_unwind_upi_lsda
+ lsda; /* address of language specific data area, */
+ /* or zero if not used */
unw_word_t __ptrauth_unwind_upi_handler_intptr handler;
- unw_word_t gp; /* not used */
- unw_word_t __ptrauth_unwind_upi_flags flags; /* not used */
- uint32_t format; /* compact unwind encoding, or zero if none */
- uint32_t unwind_info_size; /* size of DWARF unwind info, or zero if none */
- unw_word_t __ptrauth_unwind_upi_info unwind_info; /* address of DWARF unwind info, or zero */
- unw_word_t __ptrauth_unwind_upi_extra extra; /* mach_header of mach-o image containing func */
+ unw_word_t gp; /* not used */
+ unw_word_t __ptrauth_unwind_upi_flags flags; /* not used */
+ uint32_t format; /* compact unwind encoding, or zero if none */
+ uint32_t unwind_info_size; /* size of DWARF unwind info, or zero if none */
+ unw_word_t __ptrauth_unwind_upi_info
+ unwind_info; /* address of DWARF unwind info, or zero */
+ unw_word_t __ptrauth_unwind_upi_extra
+ extra; /* mach_header of mach-o image containing func */
};
typedef struct unw_proc_info_t unw_proc_info_t;
diff --git a/libunwind/src/AddressSpace.hpp b/libunwind/src/AddressSpace.hpp
index 63f9cb367..f38dda47c 100644
--- a/libunwind/src/AddressSpace.hpp
+++ b/libunwind/src/AddressSpace.hpp
@@ -129,27 +129,24 @@ struct UnwindInfoSections {
defined(_LIBUNWIND_SUPPORT_COMPACT_UNWIND) || \
defined(_LIBUNWIND_USE_DL_ITERATE_PHDR)
// No dso_base for SEH.
- uintptr_t __ptrauth_unwind_uis_dso_base
- dso_base = 0;
+ uintptr_t __ptrauth_unwind_uis_dso_base dso_base = 0;
#endif
#if defined(_LIBUNWIND_USE_DL_ITERATE_PHDR)
size_t text_segment_length;
#endif
#if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
- uintptr_t __ptrauth_unwind_uis_dwarf_section
- dwarf_section = 0;
- size_t __ptrauth_unwind_uis_dwarf_section_length
- dwarf_section_length = 0;
+ uintptr_t __ptrauth_unwind_uis_dwarf_section dwarf_section = 0;
+ size_t __ptrauth_unwind_uis_dwarf_section_length dwarf_section_length = 0;
#endif
#if defined(_LIBUNWIND_SUPPORT_DWARF_INDEX)
uintptr_t dwarf_index_section;
size_t dwarf_index_section_length;
#endif
#if defined(_LIBUNWIND_SUPPORT_COMPACT_UNWIND)
- uintptr_t __ptrauth_unwind_uis_compact_unwind_section
- compact_unwind_section = 0;
+ uintptr_t __ptrauth_unwind_uis_compact_unwind_section compact_unwind_section =
+ 0;
size_t __ptrauth_unwind_uis_compact_unwind_section_length
- compact_unwind_section_length = 0;
+ compact_unwind_section_length = 0;
#endif
#if defined(_LIBUNWIND_ARM_EHABI)
uintptr_t arm_section;
diff --git a/libunwind/src/DwarfParser.hpp b/libunwind/src/DwarfParser.hpp
index 5e494de10..19651c9c4 100644
--- a/libunwind/src/DwarfParser.hpp
+++ b/libunwind/src/DwarfParser.hpp
@@ -325,7 +325,7 @@ template <typename CIE_Info, typename T>
memmove((void *)&info->personality, (void *)&signed_personality,
sizeof(signed_personality));
}
-}
+} // namespace
/// Extract info from a CIE
template <typename A>
@@ -406,7 +406,8 @@ const char *CFI_Parser<A>::parseCIE(A &addressSpace, pint_t cie,
// could avoid this by simply giving resultAddr the correct ptrauth
// schema and performing an assignment.
const auto discriminator = ptrauth_blend_discriminator(
- &cieInfo->personality, __ptrauth_unwind_cie_info_personality_disc);
+ &cieInfo->personality,
+ __ptrauth_unwind_cie_info_personality_disc);
void *signedPtr = ptrauth_auth_and_resign(
(void *)personality, ptrauth_key_function_pointer, resultAddr,
ptrauth_key_function_pointer, discriminator);
diff --git a/libunwind/src/Registers.hpp b/libunwind/src/Registers.hpp
index 6233d70ab..1b96549ca 100644
--- a/libunwind/src/Registers.hpp
+++ b/libunwind/src/Registers.hpp
@@ -17,8 +17,8 @@
#include "config.h"
#include "libunwind.h"
-#include "shadow_stack_unwind.h"
#include "libunwind_ext.h"
+#include "shadow_stack_unwind.h"
namespace libunwind {
@@ -1839,8 +1839,8 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
public:
Registers_arm64();
Registers_arm64(const void *registers);
- Registers_arm64(const Registers_arm64&);
- Registers_arm64& operator=(const Registers_arm64&);
+ Registers_arm64(const Registers_arm64 &);
+ Registers_arm64 &operator=(const Registers_arm64 &);
typedef uint64_t reg_t;
typedef uint64_t __ptrauth_unwind_registers_arm64_link_reg link_reg_t;
@@ -1863,29 +1863,25 @@ public:
uint64_t getSP() const { return _registers.__sp; }
void setSP(uint64_t value) { _registers.__sp = value; }
- uint64_t getIP() const {
+ uint64_t getIP() const {
uint64_t value = _registers.__pc;
#if __libunwind_has_ptrauth
// Note the value of the PC was signed to its address in the register state
// but everyone else expects it to be sign by the SP, so convert on return.
- value = (uint64_t)ptrauth_auth_and_resign((void *)_registers.__pc,
- ptrauth_key_return_address,
- &_registers.__pc,
- ptrauth_key_return_address,
- getSP());
+ value = (uint64_t)ptrauth_auth_and_resign(
+ (void *)_registers.__pc, ptrauth_key_return_address, &_registers.__pc,
+ ptrauth_key_return_address, getSP());
#endif
return value;
}
- void setIP(uint64_t value) {
+ void setIP(uint64_t value) {
#if __libunwind_has_ptrauth
// Note the value which was set should have been signed with the SP.
// We then resign with the slot we are being stored in to so that both SP
// and LR can't be spoofed at the same time.
- value = (uint64_t)ptrauth_auth_and_resign((void *)value,
- ptrauth_key_return_address,
- getSP(),
- ptrauth_key_return_address,
- &_registers.__pc);
+ value = (uint64_t)ptrauth_auth_and_resign(
+ (void *)value, ptrauth_key_return_address, getSP(),
+ ptrauth_key_return_address, &_registers.__pc);
#endif
_registers.__pc = value;
}
@@ -1898,10 +1894,9 @@ public:
link_reg_t *referenceAuthedLinkRegister) {
// If we are in an arm64/arm64e frame, then the PC should have been signed
// with the SP
- *referenceAuthedLinkRegister =
- (uint64_t)ptrauth_auth_data((void *)inplaceAuthedLinkRegister,
- ptrauth_key_return_address,
- _registers.__sp);
+ *referenceAuthedLinkRegister = (uint64_t)ptrauth_auth_data(
+ (void *)inplaceAuthedLinkRegister, ptrauth_key_return_address,
+ _registers.__sp);
}
#endif
@@ -1956,11 +1951,12 @@ inline Registers_arm64::Registers_arm64(const void *registers) {
#endif
}
-inline Registers_arm64::Registers_arm64(const Registers_arm64& other) {
+inline Registers_arm64::Registers_arm64(const Registers_arm64 &other) {
*this = other;
}
-inline Registers_arm64& Registers_arm64::operator=(const Registers_arm64& other) {
+inline Registers_arm64 &
+Registers_arm64::operator=(const Registers_arm64 &other) {
memcpy(&_registers, &other._registers, sizeof(_registers));
memcpy(_vectorHalfRegisters, &other._vectorHalfRegisters,
sizeof(_vectorHalfRegisters));
diff --git a/libunwind/src/UnwindCursor.hpp b/libunwind/src/UnwindCursor.hpp
index dea7effa3..052d4c60a 100644
--- a/libunwind/src/UnwindCursor.hpp
+++ b/libunwind/src/UnwindCursor.hpp
@@ -1998,13 +1998,11 @@ bool UnwindCursor<A, R>::getInfoFromCompactEncodingSection(
#if __libunwind_has_ptrauth
// The GOT for the personality function was signed address authenticated.
// Resign it as a regular function pointer.
- const auto discriminator =
- ptrauth_blend_discriminator(&_info.handler,
- __ptrauth_unwind_upi_handler_disc);
- void *signedPtr =
- ptrauth_auth_and_resign((void *)personality, ptrauth_key_function_pointer,
- personalityPointer, ptrauth_key_function_pointer,
- discriminator);
+ const auto discriminator = ptrauth_blend_discriminator(
+ &_info.handler, __ptrauth_unwind_upi_handler_disc);
+ void *signedPtr = ptrauth_auth_and_resign(
+ (void *)personality, ptrauth_key_function_pointer, personalityPointer,
+ ptrauth_key_function_pointer, discriminator);
personality = (__typeof(personality))signedPtr;
#endif
if (log)
diff --git a/libunwind/src/UnwindLevel1.c b/libunwind/src/UnwindLevel1.c
index d924a718a..1777d6439 100644
--- a/libunwind/src/UnwindLevel1.c
+++ b/libunwind/src/UnwindLevel1.c
@@ -96,8 +96,7 @@
static _Unwind_Personality_Fn get_handler_function(unw_proc_info_t *frameInfo) {
union {
void *opaque_handler;
- _Unwind_Personality_Fn __ptrauth_unwind_upi_handler *
- handler;
+ _Unwind_Personality_Fn __ptrauth_unwind_upi_handler *handler;
} u;
u.opaque_handler = (void *)&frameInfo->handler;
return *u.handler;
diff --git a/libunwind/src/libunwind.cpp b/libunwind/src/libunwind.cpp
index 0dce8dea4..061f9917e 100644
--- a/libunwind/src/libunwind.cpp
+++ b/libunwind/src/libunwind.cpp
@@ -132,7 +132,7 @@ _LIBUNWIND_HIDDEN int __unw_set_reg(unw_cursor_t *cursor, unw_regnum_t regNum,
// It is only valid to set the IP within the current function.
// This is important for ptrauth, otherwise the IP cannot be correctly
// signed.
- [[maybe_unused]]unw_word_t stripped_value =
+ [[maybe_unused]] unw_word_t stripped_value =
(unw_word_t)ptrauth_strip((void *)value, ptrauth_key_return_address);
assert(stripped_value >= info.start_ip && stripped_value <= info.end_ip);
|
6810396
to
637245f
Compare
@kovdan01 @atrosinenko Please take a look |
Updating formatting before review - had discussed with Louis and he expressed a preference for some of it, but this llvm.org style bot complains many other cases (likely a local config issue when I was trying to cleanup the downstream code. So I've updated with a direct clang-format fix. |
unused variable errors are likely fallout from refactoring and workaround removals I did as part of the prep, will do some cleanup/diagnostics work later this week. I mostly wanted to get this available to others to see whether they were ok adopting this rather than rolling their own version. |
#if __has_feature(ptrauth_calls) | ||
retab | ||
#else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly ptrauth_returns
intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see earlier comment - do we want to just adopt a single flag instead of the current mix of ptrauth_calls and ptrauth_returns ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, if switching to ptrauth_returns
here, we most probably want to retain compatibility with Armv8.2-a by replacing
#if __has_feature(ptrauth_calls)
retab
#else
ret x30 // jump to pc
#endif
with
#if __has_feature(ptrauth_returns)
autibsp
#endif
ret x30 // jump to pc
That is, simply insert autibsp
into the existing code.
libunwind/src/UnwindRegistersSave.S
Outdated
#if __has_feature(ptrauth_calls) | ||
pacibsp | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly ptrauth_returns
intended?
libunwind/src/UnwindRegistersSave.S
Outdated
#if __has_feature(ptrauth_calls) | ||
retab | ||
#else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly ptrauth_returns
intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what we have downstream, it's possible we've just developed an assumption that they're equivalent at this point.
That said I'm actually thinking this should also be around an if __APPLE__
as well pending adoption of/a more generic gate on the ABI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here (autibsp
+ ret
instead of retab
).
libunwind/src/libunwind.cpp
Outdated
assert(stripped_value >= info.start_ip && stripped_value <= info.end_ip); | ||
#endif | ||
|
||
pint_t sp = (pint_t)co->getReg(UNW_REG_SP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pint_t sp = (pint_t)co->getReg(UNW_REG_SP); | |
pint_t sp = (pint_t)co->getReg(UNW_REG_SP); | |
(void)sp; |
This should quiet the unused variable warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to actually look at this to see if even needs to be outside of an ifdef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just removed the ifdef gap
libcxxabi/include/__cxxabi_config.h
Outdated
#define _LIBCXXABI_FUNC_VIS __declspec(dllimport) | ||
#define _LIBCXXABI_TYPE_VIS __declspec(dllimport) | ||
#endif | ||
# if defined(_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS) || \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid changing the formatting of unrelated pieces of code. It's fine if the clang-format job is not happy -- we haven't run clang-format
on libc++abi and libunwind yet, unless I mis-remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how/why/when I changed this formatting because I'd swear I went through trying to remove them all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I bet I auto-ran clang-format after the bots complained or reflexively pre-push to avoid the bots complain. Sighhhhh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I bet I auto-ran clang-format after the bots complained or reflexively pre-push to avoid the bots complain. Sighhhhh
Have you tried git clang-format <parent commit>
? I tried rolling this change back (lines 35-50), committing it and running git clang-format HEAD~2
- it seems to behave as intended: only formatting the lines affected by the commits (i.e. not re-applying this change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea how I achieved that then - I have been simultaneously rewriting downstream and merging back and forth so who knows what the source of that is going to be.
libunwind/src/Registers.hpp
Outdated
@@ -93,6 +98,13 @@ class _LIBUNWIND_HIDDEN Registers_x86 { | |||
uint32_t getEDI() const { return _registers.__edi; } | |||
void setEDI(uint32_t value) { _registers.__edi = value; } | |||
|
|||
typedef uint32_t reg_t; | |||
typedef uint32_t link_reg_t; | |||
void loadAndAuthenticateLinkRegister(reg_t srcLinkRegister, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since loadAndAuthenticateLinkRegister
is used in UnwindCursor.hpp as a member function of a registers
object (having template registers class type), we should probably consider one of the following options.
- Define
loadAndAuthenticateLinkRegister
for all the register classes. As far as I can see now, your patch contains definitions for x86, x86_64, ppc, arm64 and arm, but we also have mips, risc-v, ... - Only limit this function to arm64 and do not expose that to register classes for other architectures. In this case, you'll probably need to insert checks like
__has_feature(...)
in UnwindCursor.hpp and add special logic in these cases.
I personally prefer the 2nd option. It's not very nice, but IMHO this is better than adding a placeholder function for all the architectures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, especially as it's clearly difficult to ensure with local builds that you've got every other platform correct, and then it's make-work for every other platform.
libunwind/src/Registers.hpp
Outdated
void | ||
loadAndAuthenticateLinkRegister(reg_t inplaceAuthedLinkRegister, | ||
link_reg_t *referenceAuthedLinkRegister) { | ||
#if __has_feature(ptrauth_calls) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, you meant ptrauth_returns
. I saw your discussion with @atrosinenko regarding similar situations in assembly files and I do get the point that these were assumed identical for your purposes in downstream. But it looks like that it's time to change this to ptrauth_returns
. It's worth looking at other occurrences of ptrauth_calls
as well - at least some of them are also probably misused and should be replaced with ptrauth_returns
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use a wide array of ptrauth_returns and ptrauth_calls, do we just want a single flag/define we should use?
I think the variation we see here is the outcome of changes over time, but it would seem reasonable to maybe just have a single mode flag at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I get it right, ptrauth_returns do not influence the ABI and ptrauth_calls do, thus it may be trivial to enable pac-ret in one's own build, but ptrauth_calls have to be consistently enabled for the whole runtime.
UPD: Well, it doesn't influence ABI most of the time, otherwise we would only have to mention it around paciasp/autiasp/retaa in hand-written assembly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah, you are correct, in principle pac-ret is safely separable so yeah I'll need to go through and audit which should be gated by ptrauth_calls vs _return and which are incorrect due to being able to assume both are enabled.
libunwind/src/Registers.hpp
Outdated
@@ -1877,6 +1946,32 @@ inline Registers_arm64::Registers_arm64(const void *registers) { | |||
memcpy(_vectorHalfRegisters, | |||
static_cast<const uint8_t *>(registers) + sizeof(GPRs), | |||
sizeof(_vectorHalfRegisters)); | |||
#if __has_feature(ptrauth_calls) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably worth adding a comment explaining this piece of code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- seems reasonable
if (ptrauth_auth_and_resign((void *)pc, ptrauth_key_return_address, sp, | ||
ptrauth_key_return_address, | ||
sp) != (void *)pc) { | ||
_LIBUNWIND_LOG("Bad unwind through arm64e (0x%llX, 0x%llX)->0x%llX\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this check would be cross-platform and will do the trick for ELF platforms as well, so it's probably worth to re-phrase this and avoid arm64e term but say something in generic pauth terms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally realized why this exists - it predates the FPAC extension to ARM8.3 so authentication did not guarantee a trap at some point. Now obviously the resign would trap, so the comparison will always either pass or not be reached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While FPAC does the job and with that being enabled, we do not reach this logging, it's not true when we do not have FPAC :) We cannot guarantee that it's present. And anyway, it's better to use more generic messages in places which are not specific for arm64e.
I suggest to look through the PR and think of replacing mentions of arm64e with smth describing arbitrary ptrauth-enabled platforms (where applicable).
I can help with that when all the functional issues with this PR are resolved.
libcxxabi/src/cxa_personality.cpp
Outdated
// it impossible to directly cast them without breaking the authentication, | ||
// as a result we need this pair of helpers. | ||
template <typename PtrType> | ||
void set_landing_pad_as_ptr(scan_results& results, const PtrType& out) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PtrType
seems to always be a ... pointer type :) So, probably, you can just pass by value instead of using const l-value reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kovdan01 ah yeah, unfortunately that's not sound, but for dumb reasons. The reason I've passed by reference is because the intent is for PtrType to be a ptrauth qualified type on arm64e, and we use a reference to ensure that it remains protected.
It might be possible to make this clearer once we have the schema queries upstreamed, because then we could make it explicit that this expects PtrType to be an address discriminated __ptrauth qualified pointer when compiling on arm64e (or whatever other platforms add ptrauth in future)
@@ -207,7 +211,8 @@ bool DwarfInstructions<A, R>::isReturnAddressSignedWithPC(A &addressSpace, | |||
#endif | |||
|
|||
template <typename A, typename R> | |||
int DwarfInstructions<A, R>::stepWithDwarf(A &addressSpace, pint_t pc, | |||
int DwarfInstructions<A, R>::stepWithDwarf(A &addressSpace, | |||
typename R::link_reg_t &pc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please clarify the reason why pc
became an out-parameter? I do not see any other changes in the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kovdan01 as with the other case it's because link_reg_t is a ptrauth'd type and where intentionally keeping it protected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably make it const at least
libunwind/src/Registers.hpp
Outdated
// with the SP | ||
*referenceAuthedLinkRegister = (uint64_t)ptrauth_auth_data( | ||
(void *)inplaceAuthedLinkRegister, ptrauth_key_return_address, | ||
_registers.__sp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like that we need to use &_registers.__pc
instead of _registers.__sp
because the address was resigned in setIP
. Locally, I had an auth failure with this code, and this was resolved by using &_registers.__pc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: alternatively, instead of changing discriminator here, it's possible to change this->getReg(UNW_REG_IP)
to _registers.getIP()
in callers. This would do resigning with sp, and auth here would pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to work out whether this is me screwing up the lifting of the PR, or me screwing up the authentication changes during cleanup, or something else entirely.
@ojhunt JFYI: I've published a couple of small fixes I've applied locally at https://github.com/kovdan01/llvm-project/commits/pointer-authenticated-unwinding-fix0/. Maybe this would be useful |
I'd posted this to try and avoid too much duplicated work so it's highly possible some of these issues are due to me screwing up refactoring, cleanups, or the patch preparation. I'll get back to this after wg21 wraps up this week, so sorry for the delays. |
#include <ptrauth.h> | ||
#endif | ||
|
||
#if defined(__APPLE__) && __has_feature(ptrauth_qualifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am building this for aarch64-linux-pauthtest
now and I wonder if it is beneficial to decrease the number of hardcoded defined(__APPLE__)
guards? Something like
#if defined(__APPLE__)
#define EXTRA_PTRAUTH_HARDENING 1
#else
#define EXTRA_PTRAUTH_HARDENING 0
#endif
near the top of the file and then use EXTRA_PTRAUTH_HARDENING
throughout the code instead of defined(__APPLE__)
, so that other platforms can opt-in to this hardening by adjusting a single line.
Though, the above EXTRA_PTRAUTH_HARDENING
macro still have to be defined multiple times: separate definitions would probably be required for compiler-rt/lib/builtins/gcc_personality_v0.c
, libcxxabi
and libunwind
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I wonder if there's any existing precedence for cmake variables the apply to all the runtimes?
Also, given some of the other issues you've hit with building don't expend too much effort trying to build/test - I need to work out what parts I've managed to screw up while building out this PR - some of the issues you've hit seem likely to be me screwing up during the preparation of the PR and I don't want you folk to slog through issues caused by my muppetry
uintptr_t newIP = (uintptr_t)ptrauth_auth_and_resign( | ||
*(void **)&landingPad, ptrauth_key_function_pointer, | ||
existingDiscriminator, ptrauth_key_return_address, stack_pointer); | ||
_Unwind_SetIP(context, newIP); | ||
#else | ||
_Unwind_SetIP(context, landingPad); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to assume __has_feature(ptrauth_returns)
as well. On the other hand, I doubt we expect any valid build configuration to have __has_feature(ptrauth_calls) == true
and __has_feature(ptrauth_returns) == false
, so something as simple as
#if !__has_feature(ptrauth_returns)
#error Hardened libunwing expects pac-ret
#endif
should be perfectly enough for the sake of documentating this dependecy and just to be sure :)
But what if we build libunwind on AArch64 with pac-ret, but without extra libunwind hardening (ptrauth_calls
feature is false, unsupported platform, etc.): we probably want to manually apply a pac-ret-style protection to the argument passed to _Unwind_SetIP
(and we will probably crash at some later point if pac-ret-style protection is not applied). Probably, a simplified version of this code should be added under
_Unwind_SetIP(context, newIP);
#elif __has_feature(ptrauth_returns)
// Just sign unprotected newIP with stack pointer and pass it to _Unwind_SetIP
#else
_Unwind_SetIP(context, landingPad);
#endif
libcxxabi/src/cxa_exception.h
Outdated
void(_LIBCXXABI_DTOR_FUNC* _LIBCXXABI_PTRAUTH_FN("__cxa_exception::exceptionDestructor") | ||
exceptionDestructor)(void*); | ||
#endif | ||
std::unexpected_handler unexpectedHandler; | ||
std::terminate_handler terminateHandler; | ||
std::unexpected_handler _LIBCXXABI_PTRAUTH_FN("__cxa_exception::unexpectedHandler") unexpectedHandler; | ||
std::terminate_handler _LIBCXXABI_PTRAUTH_FN("__cxa_exception::terminateHandler") terminateHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these strings (or, strictly speaking, their hashes) part of public ABI? If so, it may be handy to put them into ptrauth.h
, like it is already done for __ptrauth_init_fini_discriminator
.
libcxxabi/src/cxa_exception.h
Outdated
const unsigned char* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::actionRecord") actionRecord; | ||
const unsigned char* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::languageSpecificData") languageSpecificData; | ||
void* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::catchTemp") catchTemp; | ||
void* _LIBCXXABI_PTRAUTH_PDD("__cxa_exception::adjustedPtr") adjustedPtr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
(unw_word_t)ptrauth_auth_and_resign(*(void**)&results.landingPad, _LIBCXXABI_PTRAUTH_KEY, existingDiscriminator, | ||
ptrauth_key_return_address, stack_pointer); | ||
_Unwind_SetIP(context, newIP); | ||
#else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in compiler-rt/lib/builtins/gcc_personality_v0.c
, do we need an implementation "in between" completely unprotected pointers and full hand-written hardening when only pac-ret is enabled?
libcxxabi/include/__cxxabi_config.h
Outdated
# include <ptrauth.h> | ||
#endif | ||
|
||
#if defined(__APPLE__) && __has_feature(ptrauth_qualifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like that __has_feature(ptrauth_qualifier)
should be replaced with __has_extension(ptrauth_qualifier)
. It is defined as EXTENSION
in clang/include/clang/Basic/Features.def, and existing tests use __has_extension
instead of __has_feature
for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigh, yup. That's why those spelling changes are so annoying :-/
I'll be back to working on this later today - there's a pile of feedback, and a bunch of nonsense that I managed to accidentally accrue and/or re-format when pushing the PR so it will take a little time to unbreak that. It may be easiest to force push a repaired PR first and then address comments. I also realize that some of the breakage might be due to mismatched APPLE vs has_feature(actual_feature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this patch on Linux with aarch64-linux-pauthtest
triple and custom-built sysroot, here are the results.
In my setup, the tests pass on the following modified branch: https://github.com/atrosinenko/llvm-project/commits/pointer-authenticated-unwinding-fix1/. It is based on https://github.com/kovdan01/llvm-project/commits/pointer-authenticated-unwinding-fix0/ with the following changes:
- Currently, several parts of the patch are only enabled on Apple targets. For testing purposes, I unconditionally enabled parts of this patch guarded by
defined(__APPLE__)
. - It turned out that
__has_feature(ptrauth_qualifier)
does not work in mainline, so I replaced it with__has_feature(ptrauth_intrinsics)
. I expect the correct solution proposed by @kovdan01 (replacing with__has_extension(ptrauth_qualifier)
) to behave identically. - The change to
compiler-rt/lib/profile/InstrProfilingValue.c
seems to be unrelated to libunwind - The most meaningful change is updating signing schemas is several places - see inline comments. Additionally, I had to update a few other places that I cannot add inline comments to
- in
Registers_arm64::getRegister
andRegisters_arm64::setRegister
, replace direct references to_registers.__pc
with calls togetIP()
andsetIP()
, correspondingly - disable the existing pauth-related logic in
DwarfInstructions<A, R>::stepWithDwarf
- in
The fact I'm worried about is whether implicit signing and authentication on accesses to __ptrauth
-qualified fields may introduce signing or authentication oracles usable by an attacker, since many values stored to these fields are initially non-signed. This is possibly mitigated by the fact that all these fields use address diversity with distinct integer discriminators and/or the original values are taken from read-only memory. On the other hand, discriminator computation, auth / sign intrinsic and load / store to memory are currently three separate operations when accessing a __ptrauth
-qualified field, thus spilling of intermediate values to the stack is possible. Furthermore, even if the non-signed value originates from a read-only memory, this is not expressed in LLVM IR terms, thus the optimization pipeline may transform sensitive instruction sequences in an unsafe way.
libcxxabi/src/cxa_personality.cpp
Outdated
const auto existingDiscriminator = ptrauth_blend_discriminator( | ||
&results.landingPad, ptrauth_string_discriminator(_LIBCXXABI_PTRAUTH_SCANRESULT_LANDINGPAD_DISC)); | ||
unw_word_t newIP = | ||
(unw_word_t)ptrauth_auth_and_resign(*(void**)&results.landingPad, _LIBCXXABI_PTRAUTH_KEY, existingDiscriminator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_LIBCXXABI_PTRAUTH_KEY
is defined to ptrauth_key_process_dependent_code
, but landingPad
field uses ptrauth_key_process_dependent_data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigh - this code was using the pointer auth schema queries and I must have screwed up while converting it, however this can be mitigated somewhat by moving a bunch of these constants into ptrauth.h rather than duplicating each locally which is really what is causing this.
&cieInfo->personality, | ||
ptrauth_string_discriminator("CIE_Info::personality")); | ||
void *signedPtr = ptrauth_auth_and_resign( | ||
(void *)personality, ptrauth_key_function_pointer, resultAddr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my configuration, I had to use
const auto oldDiscriminator = ptrauth_blend_discriminator(
(void*)resultAddr,
ptrauth_string_discriminator("personality"));
instead of plain resultAddr
. This may be pauthtest
-abi-specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be
pauthtest
-abi-specific.
@atrosinenko This actually is specific to how things work on linux when ptrauth_calls
is enabled: see #119361.
@ojhunt Could you please add the piece of code provided by @atrosinenko (see above) for non-Apple targets? See, for example, commit e2f8b9d in branch pointer-authenticated-unwinding-fix3 in my llvm-project fork.
9566f3d
to
7882528
Compare
libunwind/src/Registers.hpp
Outdated
@@ -2140,6 +2241,13 @@ class _LIBUNWIND_HIDDEN Registers_arm { | |||
uint32_t getIP() const { return _registers.__pc; } | |||
void setIP(uint32_t value) { _registers.__pc = value; } | |||
|
|||
typedef uint32_t reg_t; | |||
typedef uint32_t link_reg_t; | |||
void loadAndAuthenticateLinkRegister(reg_t srcLinkRegister, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigh I thought I had removed all of these, I think that must have been a lost change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ojhunt Thank you for the update! I like the idea of not checking for __has_include(<ptrauth.h>)
separately, as it was done in your latest version of gcc_personality_v0.c
. I wonder if other instances of #if __has_include(<ptrauth.h>)
are worth replacing with unconditionally including ptrauth.h
as soon as it is decided to use its features according to __has_feature(...)
tests (and erroring out in case this file is absent). Are there any expected use cases when absence of ptrauth.h
has to be handled gracefully?
|
||
#if __has_feature(ptrauth_qualifier) | ||
#include <ptrauth.h> | ||
#if __has_feature(ptrauth_restricted_intptr_qualifier) | ||
#define __ptrauth_gcc_personality_intptr(key, addressDiscriminated, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of not checking for __has_include(<ptrauth.h>)
separately and instead gluing #include <ptrauth.h>
unconditionally to its users, which are usually already guarded with some __has_feature(...)
test. I wonder if this pattern can be used in more places affected by this patch.
#if __has_include(<ptrauth.h>) | ||
# include <ptrauth.h> | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it is a supported configuration when ptrauth.h
header is not available, but __has_extension(ptrauth_qualifier)
is true? Note that the macro definitions guarded with #if __has_extension(ptrauth_qualifier)
still refer to identifiers like ptrauth_key_process_dependent_data
.
Maybe this could be simplified by assuming that __ptrauth_cxxabi_*
are defined to non-empty expressions if and only if __has_extension(ptrauth_qualifier)
is true, and in that case ptrauth.h
header must exist:
#if __has_extension(ptrauth_qualifier)
# include <ptrauth.h>
// ptrauth_string_discriminator("__cxa_exception::actionRecord") == 0xFC91
# define __ptrauth_cxxabi_action_record \
__ptrauth(ptrauth_key_process_dependent_data, 1, 0xFC91)
// ...
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is something I'm unsure about - there's a lot of checks for the header scattered everywhere but also plenty of places that rely on the feature existing and blindly including the header.
I'm ok putting the ptrauth.h includes unconditionally inside the feature check on the basis that if the header isn't present the ptrauth enabled paths will fail to build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ojhunt Can we get rid of __has_include(<ptrauth.h>)
check here and switch to __has_feature(ptrauth_calls) || defined(__PTRAUTH__)
just as a couple of lines below?
libcxxabi/src/cxa_exception.h
Outdated
@@ -47,10 +47,11 @@ struct _LIBCXXABI_HIDDEN __cxa_exception { | |||
// In Wasm, a destructor returns its argument | |||
void *(_LIBCXXABI_DTOR_FUNC *exceptionDestructor)(void *); | |||
#else | |||
void (_LIBCXXABI_DTOR_FUNC *exceptionDestructor)(void *); | |||
void(_LIBCXXABI_DTOR_FUNC* __ptrauth_cxxabi_exception_destructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Missing space after void
: void (_LIBCXXABI_DTOR_FUNC* ...
libcxxabi/src/cxa_exception.h
Outdated
void (_LIBCXXABI_DTOR_FUNC *exceptionDestructor)(void *); | ||
std::unexpected_handler unexpectedHandler; | ||
std::terminate_handler terminateHandler; | ||
void(_LIBCXXABI_DTOR_FUNC* __ptrauth_cxxabi_exception_destructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Missing space after void
: void (_LIBCXXABI_DTOR_FUNC* ...
Registers_arm64::reg_t oldfp = addressSpace.get64(fp); | ||
|
||
// fp points to old fp | ||
registers.setFP(oldfp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably redundant, as setFP(oldfp)
simply assigns its argument to _registers.__fp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is an area I'm unsure of - I thought the addressSpace abstraction was for cross process. ... something? But I don't know if that was just a hypothetical future concept or is actually used somewhere?
It's also possible that this is just left over from a bunch of dead code I excised while preparing this
Build failures are due to me applying some review feedback and then completely failing to actually verify things still built. #Skilled |
Ok a bunch of these errors are because clang is misdiagnosing this scenario namespace {
template <class T> void foo(T*){}
}
template <class T> void bar(T* t) {
foo(t);
} as not using the template function |
d1407d0
to
7ee3e8b
Compare
This hardens the unwinding logic and datastructures on systems that support pointer authentication. The approach taken to hardening is to harden the schemas of as many high value fields in the myriad structs as possible, and then also explicitly qualify local variables referencing privileged or security critical values. This does introduce ABI linkage between libcxx, libcxxabi, and libunwind but those are in principle separate from the OS itself so we've kept the schema definitions in the library specific headers rather than ptrauth.h
At some point I lost the changes to loadAndAuthenticateLinkRegister I also updated schema names in libunwind to be more consistent Finally while looking at the total diff I saw some places that the formatting could be improved.
7ee3e8b
to
a8e66fd
Compare
@@ -557,7 +630,21 @@ set_registers(_Unwind_Exception* unwind_exception, _Unwind_Context* context, | |||
reinterpret_cast<uintptr_t>(unwind_exception)); | |||
_Unwind_SetGR(context, __builtin_eh_return_data_regno(1), | |||
static_cast<uintptr_t>(results.ttypeIndex)); | |||
#if __has_feature(ptrauth_qualifier) | |||
auto stackPointer = _Unwind_GetGR(context, UNW_REG_SP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UNW_REG_SP
and unw_word_t
(see below) are declared in libunwind.h. While I'm not sure if including this header here is a good idea (and I see that you've removed that in e88ad93), we definitely need to provide declarations for these (otherwise we can't even compile).
Might also be related to this thread #143230 (comment)
@@ -30,6 +30,45 @@ EXCEPTION_DISPOSITION _GCC_specific_handler(PEXCEPTION_RECORD, void *, PCONTEXT, | |||
_Unwind_Personality_Fn); | |||
#endif | |||
|
|||
#if __has_feature(ptrauth_qualifier) | |||
#include <ptrauth.h> | |||
#if __has_feature(ptrauth_restricted_intptr_qualifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, in mainline clang we just can use regular __ptrauth
qualifier for integer types (support implemented in your PR #137580). Given that, can we just use the piece of code below w/o the check?
#define __ptrauth_gcc_personality_intptr(key, addressDiscriminated, \
discriminator) \
__ptrauth(key, addressDiscriminated, discriminator)
I'm not sure that I understand benefits of additional __has_feature(ptrauth_restricted_intptr_qualifier)
check, so it would be nice if you could provide an explanation why it's needed.
// it impossible to directly cast them without breaking the authentication, | ||
// as a result we need this pair of helpers. | ||
template <typename PtrType> | ||
[[maybe_unused]] void set_landing_pad_as_ptr(scan_results& results, const PtrType& out) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you've added [[maybe_unused]]
as a workaround for some kind of compiler bug in 8d620b0. I also see that you've briefly described the bug in #143230 (comment).
Do I get it right that you were not observing that locally and that was only appearing on buildbots? I tried to remove this [[maybe_unused]]
attr, and nothing changes - everything compiles perfectly w/o any related warnings.
I think it's worth submitting an issue about such mis-diagnostics if it's present (or finding an existing issue for that) and providing the link to the issue in comment so those reading the code would have an explanation for this attribute (otherwise, it's not clear why it's needed).
The same applies to get_landing_pad_as_ptr
and set_cie_info_personality
.
libunwind/src/UnwindCursor.hpp
Outdated
int stepWithDwarfFDE(bool stage2) { | ||
#if __has_extension(ptrauth_calls) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this patch, both __has_extension(ptrauth_calls)
and __has_feature(ptrauth_calls)
are used. While both work, I suggest to stick with identical style for such checks (if there are no other reasons for using different 'has extension/feature' in particular places).
Probably __has_extension(ptrauth_calls)
is more preferable since this way of doing such a check is already used in existing codebase.
Applies to this place and two other similar places in this file.
#if __has_include(<ptrauth.h>) | ||
# include <ptrauth.h> | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ojhunt Can we get rid of __has_include(<ptrauth.h>)
check here and switch to __has_feature(ptrauth_calls) || defined(__PTRAUTH__)
just as a couple of lines below?
&cieInfo->personality, | ||
ptrauth_string_discriminator("CIE_Info::personality")); | ||
void *signedPtr = ptrauth_auth_and_resign( | ||
(void *)personality, ptrauth_key_function_pointer, resultAddr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be
pauthtest
-abi-specific.
@atrosinenko This actually is specific to how things work on linux when ptrauth_calls
is enabled: see #119361.
@ojhunt Could you please add the piece of code provided by @atrosinenko (see above) for non-Apple targets? See, for example, commit e2f8b9d in branch pointer-authenticated-unwinding-fix3 in my llvm-project fork.
if (ptrauth_auth_and_resign((void *)pc, ptrauth_key_return_address, sp, | ||
ptrauth_key_return_address, | ||
sp) != (void *)pc) { | ||
_LIBUNWIND_LOG("Bad unwind through arm64e (0x%llX, 0x%llX)->0x%llX\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While FPAC does the job and with that being enabled, we do not reach this logging, it's not true when we do not have FPAC :) We cannot guarantee that it's present. And anyway, it's better to use more generic messages in places which are not specific for arm64e.
I suggest to look through the PR and think of replacing mentions of arm64e with smth describing arbitrary ptrauth-enabled platforms (where applicable).
I can help with that when all the functional issues with this PR are resolved.
ldr x16, [x0, #0x0F8] // load sp into scratch | ||
ldr lr, [x0, #0x100] // restore pc into lr | ||
|
||
#if __has_feature(ptrauth_calls) || defined(__PTRAUTH__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a correct check? It looks like that #if __has_feature(ptrauth_returns)
(w/o check against __PTRAUTH__
) is more suitable, isn't it? __PTRAUTH__
just ensures that ptrauth intrinsics and header file are available, while here we want to ensure that RA is signed, don't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot add inline comment, so commenting on the whole file: stepWithDwarf
function contains piece of logic for handling signed RA. This works properly for pac-ret, but it interferes with logic for ptrauth-returns which is a part of pauthtest ABI on Linux and a part of Apple's arm64e. So, it worth to disable this piece of logic when ptrauth-returns is present - see, for example, commit 0e8f327 in my branch pointer-authenticated-unwinding-fix3.
Alternatively, we can change the logic in a way it handles both pac-ret and ptrauth-returns properly w/o interfering (not sure if it's worth doing that though).
P.S. The proof-of-concept fix was initially implemented by @atrosinenko in commit 9c995f7 in his branch pointer-authenticated-unwinding-fix1
Thanks @ojhunt for updates! These resolve some of the previously found issues, but many problems are still present (both previously reported and newly found). I tried to resolve all the threads which are now no longer relevant, so please treat all the opened threads as issues which need your attention. I would be happy to provide any help - just let me know if it's needed. Here are the issues I'd want to highlight - these should probably be resolved first.
|
This hardens the unwinding logic and datastructures on systems
that support pointer authentication.
The approach taken to hardening is to harden the schemas of as many
high value fields in the myriad structs as possible, and then also
explicitly qualify local variables referencing privileged or security
critical values.
This does introduce ABI linkage between libcxx, libcxxabi, and
libunwind but those are in principle separate from the OS itself
so we've kept the schema definitions in the library specific headers
rather than ptrauth.h