Skip to content

[runtimes][PAC] Harden unwinding when possible (#138571) #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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[runtimes][PAC] Harden unwinding when possible (#138571) #143230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 63 additions & 6 deletions compiler-rt/lib/builtins/gcc_personality_v0.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,46 @@ 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)
Copy link
Contributor

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.

Copy link
Contributor Author

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

#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

Expand Down Expand Up @@ -205,7 +245,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.
Expand All @@ -224,11 +265,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.
Expand All @@ -238,7 +282,20 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] It seems like stackPointer better aligns with the prevailing naming style of this file.

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
Comment on lines +292 to +298
Copy link
Contributor

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

return _URC_INSTALL_CONTEXT;
}
}
Expand Down
8 changes: 7 additions & 1 deletion compiler-rt/lib/profile/InstrProfilingValue.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

VALID_CODE_KEY is not defined, have you forgotten that? In some tests, there is #define VALID_CODE_KEY 0 definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible this is a mis-merge/copy - I'll try to work out where VALID_CODE_KEY is expected to come from and ensure that I pull that def in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this entire change to compiler-rt/lib/profile/InstrProfilingValue.c may be unrelated to libunwind.

Copy link
Contributor

Choose a reason for hiding this comment

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

In order to use ptrauth_sign_unauthenticated, you need to include ptrauth.h:

#if __has_include(<ptrauth.h>)
#include <ptrauth.h>
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, I'll see if this actually caused any of the bot failures, and if it's not responsible for any I'll try to work out why

@kovdan01 @asl do you folk have a vm setup you can test on? I realize I'm currently limited to building for darwin so it's easy for me to luck out on implicit/transitive includes from other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kovdan01 @asl do you folk have a vm setup you can test on? I realize I'm currently limited to building for darwin so it's easy for me to luck out on implicit/transitive includes from other places.

@ojhunt Yeah, we have that and we can run llvm-test-suite and some other private pauth-specific tests. And, of course, we have a build of pauth-enabled linux toolchain configured.

Basically, having these is the reason why I'm able to add such comments in this PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kovdan01 I mean "do you have step by step instructions for a muppet like me that would make it possible for me to build/test locally" :D

Copy link
Contributor

Choose a reason for hiding this comment

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

@kovdan01 I mean "do you have step by step instructions for a muppet like me that would make it possible for me to build/test locally" :D

@ojhunt Ah, thanks for clarifying your question :) Unfortunately, right now our build scripts, instructions, tests, ... are not available publicly. We'll definitely publish them, but right now I guess that we would be happy to test your changes on our side manually. This would, of course, slow down the work on the PR, but at this point it looks like that many of the comments we left actually are relevant for all targets, not only for linux. So, at least for such comments you'll be able to test things using your system w/o our linux-specific stuff.

#endif
return FP;
}

/* Allocate an array that holds the pointers to the linked lists of
Expand Down
67 changes: 51 additions & 16 deletions libcxxabi/include/__cxxabi_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,23 @@
#endif

#if defined(_WIN32)
#if defined(_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS) || (defined(__MINGW32__) && !defined(_LIBCXXABI_BUILDING_LIBRARY))
#define _LIBCXXABI_HIDDEN
#define _LIBCXXABI_DATA_VIS
#define _LIBCXXABI_FUNC_VIS
#define _LIBCXXABI_TYPE_VIS
#elif defined(_LIBCXXABI_BUILDING_LIBRARY)
#define _LIBCXXABI_HIDDEN
#define _LIBCXXABI_DATA_VIS __declspec(dllexport)
#define _LIBCXXABI_FUNC_VIS __declspec(dllexport)
#define _LIBCXXABI_TYPE_VIS __declspec(dllexport)
#else
#define _LIBCXXABI_HIDDEN
#define _LIBCXXABI_DATA_VIS __declspec(dllimport)
#define _LIBCXXABI_FUNC_VIS __declspec(dllimport)
#define _LIBCXXABI_TYPE_VIS __declspec(dllimport)
#endif
# if defined(_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS) || \
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

(defined(__MINGW32__) && !defined(_LIBCXXABI_BUILDING_LIBRARY))
# define _LIBCXXABI_HIDDEN
# define _LIBCXXABI_DATA_VIS
# define _LIBCXXABI_FUNC_VIS
# define _LIBCXXABI_TYPE_VIS
# elif defined(_LIBCXXABI_BUILDING_LIBRARY)
# define _LIBCXXABI_HIDDEN
# define _LIBCXXABI_DATA_VIS __declspec(dllexport)
# define _LIBCXXABI_FUNC_VIS __declspec(dllexport)
# define _LIBCXXABI_TYPE_VIS __declspec(dllexport)
# else
# define _LIBCXXABI_HIDDEN
# define _LIBCXXABI_DATA_VIS __declspec(dllimport)
# define _LIBCXXABI_FUNC_VIS __declspec(dllimport)
# define _LIBCXXABI_TYPE_VIS __declspec(dllimport)
# endif
#else
#if !defined(_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS)
#define _LIBCXXABI_HIDDEN __attribute__((__visibility__("hidden")))
Expand Down Expand Up @@ -109,4 +110,38 @@
# 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
32 changes: 18 additions & 14 deletions libcxxabi/src/cxa_exception.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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* _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;
Comment on lines +50 to +54
Copy link
Contributor

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.


__cxa_exception *nextException;

Expand All @@ -61,10 +62,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;
Comment on lines +65 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

#endif

#if !defined(__LP64__) && !defined(_WIN64) && !defined(_LIBCXXABI_ARM_EHABI)
Expand All @@ -79,16 +80,19 @@ 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.
void* primaryException;
#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;

Expand All @@ -99,10 +103,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)
Expand Down
66 changes: 58 additions & 8 deletions libcxxabi/src/cxa_personality.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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)

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,
Expand All @@ -557,7 +596,19 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Possibly incorrect naming style: stack_pointer (though, names in this file doesn't seem very consistent anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I spent a bunch of time trying to get the spellings more consistent but I really wasn't sure :-/

// 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
Copy link
Contributor

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?

_Unwind_SetIP(context, results.landingPad);
#endif
}

/*
Expand Down Expand Up @@ -691,12 +742,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__
Expand Down Expand Up @@ -935,8 +986,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.
Expand Down Expand Up @@ -970,7 +1020,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
Expand Down
Loading
Loading