[Darwin] Improve ASan diagnostics on arm64e with pointer auth

[yln]

Summary

When reporting diagnostics from ASan's (and other sanitizer's) signal
handlers we should strip the "invalid signature" bit before printing
addresses. This makes the report less confusing and let's the user
focus on the real issue.

rdar://62615826

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

783

arm_thread_state64_ptrauth_strip would be the best tool for the job here, but we can't rely on it since only the newest SDKs define it.

812

I feel that this is not the natural place to strip addr. On the other hand, it requires the least amount of changes. I have considered these other places:

• HandleDeadlySignal() before we create the SignalContext. I decided against it because it is defined in sanitizer_symbolizer_report.cpp (a platform-independent file).
• SignalContext::GetAddress() in sanitizer_posix.cpp.
  • We could create Mac and Linux variants of this function , or
  • use additional #ifdefs in the _posix file.

Let me know which version you prefer.

1149

Drive-by fix to nicely align the registers printed via DUMPREGA64.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

771

Maybe this should be?

#if  defined(__arm64e__)
#  if __has_feature(ptrauth_calls)
#    include <ptrauth.h>
#  else
#    error "Compiling for arm64e but toolchain is missing ptrauth_calls"
#  endif
#else
#  if __has_feature(ptrauth_calls)
#     # error "ptrauth_calls on a non arm64e target is not supported"
#  endif
#define ptrauth_strip(value, key) (value)
#endif

I'm not sure if this is necessary because ptrauth_calls is part of the toolchain and not the SDK. So these additional checks only make sense if the AppleClang can target arm64e without the ptrauth_calls enabled.

777

@yln Is this the right place for the change? My reading of this is that this macro gets used for the pc, fp, lr, and sp registers. I would expect the contents of the pc and lr registers to be signed but I'm not sure about the fp and sp registers.

812

It does seems a little weird to be changing addr in InitPcSpBp because the name sounds like it doesn't touch addr.

Doing it in SignalContext::GetAddress() in sanitizer_posix.cpp sounds like the right thing to do. Rather than ifdef'ing the definition of the function it might be simpler to introduce a __sanitizer::ptr_auth_strip inline function in a new header file and have SignalContext::GetAddress() unconditionally use it. This new function would just return its input where the platform doesn't do ptr auth. This has the added benefit of introducing a header file we can use from multiple translation units to do ptr auth stuff. This would hopefully mean we could keep all the ifdef'ing contained to that single header file.

What do you think? If you like this idea then it probably needs to a separate change as you'll be touching non-darwin code paths. We could land this change and have two follow up patches, one that introduces the new header file and then a second that changes sanitizer_mac.cpp to use it.

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

771

Ideally, we would just #include <ptrauth.h> unconditionally. It defines the macros on all platforms. The reason it is guarded with an #ifdef is that the header is fairly new and we want to be able to build with older SDKs.

Why __has_feature(ptrauth_calls) and not __has_include(<ptrauth.h>)?
I don't have a good answer for this, but that's the pattern have seen Kuba and others use.
@kubamracek Can you explain?

777

arm_thread_state64_ptrauth_strip strips all 4 registers. For ptrauth_* operations we usually have to be careful what pointer type (code/data/process-[in]dependent) we are working with and choose the right key. strip seems to always work with key 0.
My understanding is that strip just zeroes the "signature bits", i.e., an unsigned pointer isn't changed.

812

Sounds good. As a follow-up I will create a new header that essentially does what <ptrauth.h> does: define the macros as nops on platforms without ptrauth, so we can avoid excessive #ifdefing.

[delcypher]

LGTM provided we introduced the discussed header in future patches.

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

771

@yln Well the header exists in the toolchain and that toolchain can build for multiple targets, not just arm64e. Presumably the header still is available when arm64e is not the target and that's why __has_feature(ptrauth_calls) is needed too?

812

@yln I'd prefer an inline function in the proposed header rather than a macro but we can discuss this more when you put up the patch.

[thakis]

Given that this file is in lib/clang/11.0.3/include/ptrauth.h in Xcode, are there plans to upstream ptrauth.h too?

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

 //===-- sanitizer_mac.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 }
 
 bool SignalContext::IsTrueFaultingAddress() const {
   auto si = static_cast<const siginfo_t *>(siginfo);
   // "Real" SIGSEGV codes (e.g., SEGV_MAPERR, SEGV_MAPERR) are non-zero.
   return si->si_signo == SIGSEGV && si->si_code != 0;
 }
 
+#if __has_feature(ptrauth_calls)
+# include <ptrauth.h>
+#else
+# define ptrauth_strip(value, key) (value)
+#endif
+
 #if defined(__aarch64__) && defined(arm_thread_state64_get_sp)
   #define AARCH64_GET_REG(r) \
-    arm_thread_state64_get_##r(ucontext->uc_mcontext->__ss)
+    (uptr)ptrauth_strip(     \
+        (void *)arm_thread_state64_get_##r(ucontext->uc_mcontext->__ss), 0)
 #else
   #define AARCH64_GET_REG(r) ucontext->uc_mcontext->__ss.__##r
 #endif
 
 static void GetPcSpBp(void *context, uptr *pc, uptr *sp, uptr *bp) {
   ucontext_t *ucontext = (ucontext_t*)context;
 # if defined(__aarch64__)
   *pc = AARCH64_GET_REG(pc);
 #   if defined(__IPHONE_8_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_8_0
   *bp = AARCH64_GET_REG(fp);
 #   else
   *bp = AARCH64_GET_REG(lr);
 #   endif
   *sp = AARCH64_GET_REG(sp);
# elif defined(__i386__)
   *pc = ucontext->uc_mcontext->__ss.__eip;
   *bp = ucontext->uc_mcontext->__ss.__ebp;
   *sp = ucontext->uc_mcontext->__ss.__esp;
 # else
 # error "Unknown architecture"
 # endif
 }
 
-void SignalContext::InitPcSpBp() { GetPcSpBp(context, &pc, &sp, &bp); }
+void SignalContext::InitPcSpBp() {
+  addr = (uptr)ptrauth_strip((void *)addr, 0);
+  GetPcSpBp(context, &pc, &sp, &bp);
+}
 
 void InitializePlatformEarly() {
   // Only use xnu_fast_mmap when on x86_64 and the OS supports it.
   use_xnu_fast_mmap =
 #if defined(__x86_64__)
       GetMacosVersion() >= MACOS_VERSION_HIGH_SIERRA_DOT_RELEASE_4;
 #else
       false;

 void GetMemoryProfile(fill_profile_f cb, uptr *stats, uptr stats_size) { }
 
 void SignalContext::DumpAllRegisters(void *context) {
   Report("Register values:\n");
 
   ucontext_t *ucontext = (ucontext_t*)context;
 # define DUMPREG64(r) \
     Printf("%s = 0x%016llx  ", #r, ucontext->uc_mcontext->__ss.__ ## r);
 # define DUMPREGA64(r) \
-    Printf("%s = 0x%016llx  ", #r, AARCH64_GET_REG(r));
+    Printf("   %s = 0x%016llx  ", #r, AARCH64_GET_REG(r));
 # define DUMPREG32(r) \
     Printf("%s = 0x%08x  ", #r, ucontext->uc_mcontext->__ss.__ ## r);
 # define DUMPREG_(r)   Printf(" "); DUMPREG(r);
 # define DUMPREG__(r)  Printf("  "); DUMPREG(r);
 # define DUMPREG___(r) Printf("   "); DUMPREG(r);
 
 # if defined(__x86_64__)
 #  define DUMPREG(r) DUMPREG64(r)