[AArch64][compiler-rt] Strip PAC from the link register.

[danielkiss]

Summary

-mbranch-protection protects the LR on the stack with PAC.
When the frames are walked the LR need to be cleared.
This inline assembly later will be replaced with a new builtin.

Test: build with -DCMAKE_C_FLAGS="-mbranch-protection=standard".

[kubamracek]

Could we reuse the macros/builtins from https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h instead? Something like this:

     // Let's assume that any pointer in the 0th page (i.e. <0x1000 on i386 and
     // x86_64) is invalid and stop unwinding here.  If we're adding support for
     // a platform where this isn't true, we need to reconsider this check.
     if (pc1 < kPageSize)
       break;
+#if __has_feature(ptrauth_returns)
+    pc1 = (uhwptr)ptrauth_strip((void *)pc1, ptrauth_key_return_address);
+#endif

?

[danielkiss]

In D98008#2607313, @kubamracek wrote:

Could we reuse the macros/builtins from https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h instead?

Yes, that is the plan "This inline assembly later will be replaced with a new builtin." I tried to refer to this builtin.
Right now the intrinsics are not jet made into the codebase: D90868/D91087. Maybe hight time to review/merge them?

[danielkiss]

Keep the PAC in the stack trace and only strip it when necessary like comparing traces, looking up symbols.
Moved the strip to the sanitizer_ptrauth.h.

[kristof.beyls]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

39

This is probably bikeshedding, but to me, STRIP_PAC would be a more meaningful name than STRIP_PC.

[kubamracek]

Keep the PAC in the stack trace and only strip it when necessary like comparing traces, looking up symbols.

Interesting, I'm not saying it's necessarily a bad idea, but could you explain the motivation? The previous approach (stripping the stack trace PCs at the point of capturing stack trace) seemed simpler, easier to implement and presumably faster (less stripping operations overall).

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

39

Could we do at least STRIP_PAC_PC, or go back to STRIP_PC? The idea is that stripping PCs is not necessarily the same operation as stripping frame pointers, function pointers, or other types.

[danielkiss]

In D98008#2614592, @kubamracek wrote:

Keep the PAC in the stack trace and only strip it when necessary like comparing traces, looking up symbols.

Interesting, I'm not saying it's necessarily a bad idea, but could you explain the motivation? The previous approach (stripping the stack trace PCs at the point of capturing stack trace) seemed simpler, easier to implement and presumably faster (less stripping operations overall).

Storing the PAC protected return addresses sounds safer because the values in trace can't be used directly as jump addresses. Assuming some might use sanitisers in production.

[danielkiss]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

39

__builtin_extract_return_address could do this but back then the decision was to strip return address in __builtin_return_address which is generally fine. both would be useful in this case.

STRIP_PAC_PC sounds good to me,

[danielkiss]

The current think is that the unstripped value of the return addresses is not too problematic.
Let's keep it simple now.

[danielkiss]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

23

using the hint because some build bot are building it with clang9/10 and those does not accept the mnemonic without v8.3 instruction set.

39

I keep STRIP_PC because as it is used today it is fine in the tsan context.

[danielkiss]

fix for lint

[kubamracek]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

23

Even a "hint" I think should really be #ifdef'd under something more specific than just __aarch64__. Do you have any macro from Clang that would tell when we're targeting a PAC-enabled target?

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp

126

How about:

1. moving this below the kPageSize check (because that check is trying to detect non-pointers and garbage values and sentinels which are not ptrauth'd)
2. making the strip call unconditional (it's no-op on non-PAC targets)
3. using the handy STRIP_PC macro instead of ptrauth_strip?

[danielkiss]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

23

There is one, the __ARM_FEATURE_PAC_DEFAULT.
The slight problem it is that it is set only when the code is compiled with -mbranch-protection=*.
Today the runtime is compiled without it on most of platforms but the user's code might compiled with it so there is a fairly hight chance the runtime will meet with a signed LR.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp

126

1. Let's consider this scenario. LR could be set to null in assembly before the function is called because no return is expected. The called PAC-RET protected function will store the signed null on the stack. Without stripping the below check will fail because of the PAC bits. If the value is invalid the strip will leave it as is. I think one of the reasons of the below check is to catch the null pointers.

2,3: Will do.

[kubamracek]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

23

@ab what do you say to this?

In my opinion, we do *not* want these instructions to end up in the runtime when building for "plain aarch64", even if the instructions are just HINTs that don't do anything. At a minimum, we would need an !APPLE or something like that.

[pbarrio]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

23

I would expect some users to want universal binaries that can run on both base AArch64 and PAC-enabled systems. This is also meant for systems other than Apple, so I think !APPLE wouldn't work. What is the reason behind not wanting HINTs in plain AArch64? Is it just code size? Do you see any security implication?

[danielkiss]

Implementing the review comment.

[danielkiss]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

23

In my use case the runtime is built with pac-ret so
__ARM_FEATURE_PAC_DEFAULT work for me.
Generally this problem of PAC/NonPAC compatible runtime support need to be solved in each runtime component I think we could defer the solution.

[kubamracek]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

14

Also add && !__APPLE__ please. I think there's no good reason to change the behavior of PAC on Apple platforms, so we shouldn't do that.

[kubamracek]

LGTM, and thanks!

[kubamracek]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

39

The extra cast, (uptr)pc, is causing downstream failures for the Apple PAC implementation. The ptrauth_strip builtin expects to receive pointers only — could you drop this (uptr)pc cast (and I guess change your ptrauth_strip to also expect a pointer)?

[danielkiss]

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

39

Sorry for that. fixed in reland: 4220531ceff0742851b8a2a5836400a7a521491d

compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h

 //===-- sanitizer_ptrauth.h -------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SANITIZER_PTRAUTH_H
 #define SANITIZER_PTRAUTH_H
 
 #if __has_feature(ptrauth_calls)
 #include <ptrauth.h>
+#elif defined(__ARM_FEATURE_PAC_DEFAULT) && !defined(__APPLE__)
+inline unsigned long ptrauth_strip(unsigned long __value, unsigned int __key) {
+  // On the stack the link register is protected with Pointer
+  // Authentication Code when compiled with -mbranch-protection.
+  // Let's stripping the PAC unconditionally because xpaclri is in
+  // the NOP space so will do nothing when it is not enabled or not available.
+  unsigned long ret;
+  asm volatile(
+      "mov x30, %1\n\t"
+      "hint #7\n\t"  // xpaclri
+      "mov %0, x30\n\t"
+      : "=r"(ret)
+      : "r"(__value)
+      : "x30");
+  return ret;
+}
+#define ptrauth_auth_data(__value, __old_key, __old_data) __value
+#define ptrauth_string_discriminator(__string) ((int)0)
 #else
 // Copied from <ptrauth.h>
 #define ptrauth_strip(__value, __key) __value
 #define ptrauth_auth_data(__value, __old_key, __old_data) __value
 #define ptrauth_string_discriminator(__string) ((int)0)
 #endif
 
-#define STRIP_PC(pc) ((uptr)ptrauth_strip(pc, 0))
+#define STRIP_PAC_PC(pc) ((uptr)ptrauth_strip((uptr)pc, 0))
 
 #endif // SANITIZER_PTRAUTH_H

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp

 // run-time libraries.
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_stacktrace.h"
 
 #include "sanitizer_common.h"
 #include "sanitizer_flags.h"
 #include "sanitizer_platform.h"
+#include "sanitizer_ptrauth.h"
 
 namespace __sanitizer {
 
 uptr StackTrace::GetNextInstructionPc(uptr pc) {
 #if defined(__sparc__) || defined(__mips__)
   return pc + 8;
 #elif defined(__powerpc__) || defined(__arm__) || defined(__aarch64__)
   return pc + 4;
if (!IsValidFrame((uptr)caller_frame, stack_top, bottom) ||
       break;
     uhwptr pc1 = caller_frame[2];
 #elif defined(__s390__)
     uhwptr pc1 = frame[14];
 #elif defined(__riscv)
     // frame[-1] contains the return address
     uhwptr pc1 = frame[-1];
 #else
-    uhwptr pc1 = frame[1];
+    uhwptr pc1 = STRIP_PAC_PC(frame[1]);
 #endif
     // Let's assume that any pointer in the 0th page (i.e. <0x1000 on i386 and
     // x86_64) is invalid and stop unwinding here.  If we're adding support for
     // a platform where this isn't true, we need to reconsider this check.
     if (pc1 < kPageSize)
       break;
     if (pc1 != pc) {
       trace_buffer[size++] = (uptr) pc1;

compiler-rt/lib/tsan/rtl/tsan_external.cpp

void __tsan_external_assign_tag(void *addr, void *tag) {
   }
   if (b) {
     b->tag = (uptr)tag;
   }
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_read(void *addr, void *caller_pc, void *tag) {
-  ExternalAccess(addr, STRIP_PC(caller_pc), tag, MemoryRead);
+  ExternalAccess(addr, STRIP_PAC_PC(caller_pc), tag, MemoryRead);
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_write(void *addr, void *caller_pc, void *tag) {
-  ExternalAccess(addr, STRIP_PC(caller_pc), tag, MemoryWrite);
+  ExternalAccess(addr, STRIP_PAC_PC(caller_pc), tag, MemoryWrite);
 }
 }  // extern "C"
 
 #endif  // !SANITIZER_GO
 
 }  // namespace __tsan

compiler-rt/lib/tsan/rtl/tsan_interface.cpp

 }
 
 void __tsan_write16(void *addr) {
   MemoryWrite(cur_thread(), CALLERPC, (uptr)addr, kSizeLog8);
   MemoryWrite(cur_thread(), CALLERPC, (uptr)addr + 8, kSizeLog8);
 }
 
 void __tsan_read16_pc(void *addr, void *pc) {
-  MemoryRead(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog8);
-  MemoryRead(cur_thread(), STRIP_PC(pc), (uptr)addr + 8, kSizeLog8);
+  MemoryRead(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog8);
+  MemoryRead(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr + 8, kSizeLog8);
 }
 
 void __tsan_write16_pc(void *addr, void *pc) {
-  MemoryWrite(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog8);
-  MemoryWrite(cur_thread(), STRIP_PC(pc), (uptr)addr + 8, kSizeLog8);
+  MemoryWrite(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog8);
+  MemoryWrite(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr + 8, kSizeLog8);
 }
 
 // __tsan_unaligned_read/write calls are emitted by compiler.
 
 void __tsan_unaligned_read2(const void *addr) {
   UnalignedMemoryAccess(cur_thread(), CALLERPC, (uptr)addr, 2, false, false);
 }
 

compiler-rt/lib/tsan/rtl/tsan_interface_inl.h

void __tsan_write4(void *addr) {
   MemoryWrite(cur_thread(), CALLERPC, (uptr)addr, kSizeLog4);
 }
 
 void __tsan_write8(void *addr) {
   MemoryWrite(cur_thread(), CALLERPC, (uptr)addr, kSizeLog8);
 }
 
 void __tsan_read1_pc(void *addr, void *pc) {
-  MemoryRead(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog1);
+  MemoryRead(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog1);
 }
 
 void __tsan_read2_pc(void *addr, void *pc) {
-  MemoryRead(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog2);
+  MemoryRead(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog2);
 }
 
 void __tsan_read4_pc(void *addr, void *pc) {
-  MemoryRead(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog4);
+  MemoryRead(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog4);
 }
 
 void __tsan_read8_pc(void *addr, void *pc) {
-  MemoryRead(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog8);
+  MemoryRead(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog8);
 }
 
 void __tsan_write1_pc(void *addr, void *pc) {
-  MemoryWrite(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog1);
+  MemoryWrite(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog1);
 }
 
 void __tsan_write2_pc(void *addr, void *pc) {
-  MemoryWrite(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog2);
+  MemoryWrite(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog2);
 }
 
 void __tsan_write4_pc(void *addr, void *pc) {
-  MemoryWrite(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog4);
+  MemoryWrite(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog4);
 }
 
 void __tsan_write8_pc(void *addr, void *pc) {
-  MemoryWrite(cur_thread(), STRIP_PC(pc), (uptr)addr, kSizeLog8);
+  MemoryWrite(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, kSizeLog8);
 }
 
 void __tsan_vptr_update(void **vptr_p, void *new_val) {
   CHECK_EQ(sizeof(vptr_p), 8);
   if (*vptr_p != new_val) {
     ThreadState *thr = cur_thread();
     thr->is_vptr_access = true;
     MemoryWrite(thr, CALLERPC, (uptr)vptr_p, kSizeLog8);
     thr->is_vptr_access = false;
   }
 }
 
 void __tsan_vptr_read(void **vptr_p) {
   CHECK_EQ(sizeof(vptr_p), 8);
   ThreadState *thr = cur_thread();
   thr->is_vptr_access = true;
   MemoryRead(thr, CALLERPC, (uptr)vptr_p, kSizeLog8);
   thr->is_vptr_access = false;
 }
 
 void __tsan_func_entry(void *pc) {
-  FuncEntry(cur_thread(), STRIP_PC(pc));
+  FuncEntry(cur_thread(), STRIP_PAC_PC(pc));
 }
 
 void __tsan_func_exit() {
   FuncExit(cur_thread());
 }
 
 void __tsan_ignore_thread_begin() {
   ThreadIgnoreBegin(cur_thread(), CALLERPC);
 }
 
 void __tsan_ignore_thread_end() {
   ThreadIgnoreEnd(cur_thread(), CALLERPC);
 }
 
 void __tsan_read_range(void *addr, uptr size) {
   MemoryAccessRange(cur_thread(), CALLERPC, (uptr)addr, size, false);
 }
 
 void __tsan_write_range(void *addr, uptr size) {
   MemoryAccessRange(cur_thread(), CALLERPC, (uptr)addr, size, true);
 }
 
 void __tsan_read_range_pc(void *addr, uptr size, void *pc) {
-  MemoryAccessRange(cur_thread(), STRIP_PC(pc), (uptr)addr, size, false);
+  MemoryAccessRange(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, size, false);
 }
 
 void __tsan_write_range_pc(void *addr, uptr size, void *pc) {
-  MemoryAccessRange(cur_thread(), STRIP_PC(pc), (uptr)addr, size, true);
+  MemoryAccessRange(cur_thread(), STRIP_PAC_PC(pc), (uptr)addr, size, true);
 }