Page MenuHomePhabricator

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

Authored by danielkiss on Mar 4 2021, 11:08 PM.

Details

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".

Diff Detail

Event Timeline

danielkiss created this revision.Mar 4 2021, 11:08 PM
danielkiss requested review of this revision.Mar 4 2021, 11:08 PM
kubamracek added a reviewer: ab.

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

?

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 updated this revision to Diff 329296.Mar 9 2021, 4:45 AM

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.

danielkiss planned changes to this revision.Mar 9 2021, 6:03 AM
kristof.beyls added inline comments.Mar 9 2021, 7:31 AM
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.

danielkiss marked an inline comment as done.

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.

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 added inline comments.Mar 9 2021, 11:55 AM
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,

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

danielkiss marked an inline comment as done.Mar 11 2021, 7:46 AM
danielkiss added inline comments.
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.

fix for lint

kubamracek added inline comments.Mar 11 2021, 9:47 AM
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 added inline comments.Mar 11 2021, 11:51 AM
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 added inline comments.Mar 11 2021, 4:49 PM
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 added inline comments.Mar 12 2021, 2:04 AM
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?

Implementing the review comment.

danielkiss added inline comments.Mar 12 2021, 11:44 AM
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 added inline comments.Mar 12 2021, 12:47 PM
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.

danielkiss marked an inline comment as done.
kubamracek accepted this revision.Mar 12 2021, 5:36 PM

LGTM, and thanks!

This revision is now accepted and ready to land.Mar 12 2021, 5:36 PM
This revision was landed with ongoing or failed builds.Mar 15 2021, 2:26 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 2:26 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
kubamracek added inline comments.Mar 18 2021, 8:50 AM
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 marked an inline comment as done.Mar 18 2021, 2:06 PM
danielkiss added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_ptrauth.h
39

Sorry for that. fixed in reland: 4220531ceff0742851b8a2a5836400a7a521491d