Page MenuHomePhabricator

[clang] Add support for the new pointer authentication builtins.
Needs ReviewPublic

Authored by ab on Nov 1 2021, 11:12 AM.

Details

Summary

Building on D90868, this defines the basic set of pointer authentication clang builtins (provided in a new header, ptrauth.h), with diagnostics and IRGen support.
The availability of the builtins is gated on a new flag -fptrauth-intrinsics, which is enabled by default by the driver for darwin arm64e.

Note that this only includes the basic intrinsics (matching D90868), and notably excludes ptrauth_sign_constant, and ptrauth_type_discriminator/ptrauth_string_discriminator, which need extra logic to be fully supported. If it helps, I can bring the header/sema support here so that we can review all builtins together, and do full IRGen later.

Diff Detail

Event Timeline

ab created this revision.Nov 1 2021, 11:12 AM
ab requested review of this revision.Nov 1 2021, 11:12 AM
apazos added inline comments.Nov 2 2021, 4:13 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
839

These two error types are confusing.
In which situation would err_ptrauth_disabled be printed?
With this patch, it is only supported with arm64e triple, all other targets it is unsupported.

apazos added a comment.Nov 2 2021, 4:14 PM

The plan to push support for ptrauth_sign_constant, and ptrauth_type_discriminator/ptrauth_string_discriminator in a separate patch is good.
This current patch is already big.

Mostly LGTM, although I am not the most unbiased reviewer. :)

clang/include/clang/Basic/DiagnosticSemaKinds.td
839

We could probably just have one of these, yes.

clang/lib/Basic/Targets/AArch64.cpp
844

There's an LLVM constants header for this now, right?

The original ptrauth.h has the same comment style. Would doxygen style be an improvement?

ab updated this revision to Diff 384825.Nov 4 2021, 11:28 AM
ab marked 3 inline comments as done.

Simplify err_ptrauth_disabled diagnostic, rebase.

ab updated this revision to Diff 384827.Nov 4 2021, 11:29 AM

The original ptrauth.h has the same comment style. Would doxygen style be an improvement?

Hmm, what do you have in mind? Markup for the builtin arguments/returns? The need for __ prefixes makes that a bit awkward, but I suppose that's not a big deal (and we do live with that for x86 intrins already)

clang/include/clang/Basic/DiagnosticSemaKinds.td
839

With this patch you'd get err_ptrauth_disabled with -target arm64e- -fno-ptrauth-intrinsics, instead of err_ptrauth_disabled_target with say -target x86_64. But yeah, I suppose we could live with only one of the two, with a less accurate message (either err_ptrauth_disabled_target renamed to err_ptrauth_disabled, or another more vague message altogether, say "pointer authentication is not supported")

This mostly removes the need for Sema::diagnosePointerAuthDisabled and TargetInfo::isPointerAuthSupported and lets us just rely on the individual langopts, which seems okay.

clang/lib/Basic/Targets/AArch64.cpp
844

Hmm, I don't think so, do you have something in mind? The AArch64 backend has helpers for this, but that's confined there. IR doesn't know anything about keys (for instance, the IR verifier doesn't have the same validation done here)

There's a related latent concern I have: currently, nothing prevents optimizations from moving ptrauth ops into global constant initializers, and since we don't support global initializers with process-specific keys (at least on darwin), we may need to expose this knowledge somehow (undoing this in the backend is not straightforward, but is an option).

Knowing which keys are used in what way would need to be triple-specific, though I guess that's not an obstacle.

The avxintrin.h header has more structured documentation.

kristof.beyls added inline comments.Nov 5 2021, 3:07 AM
clang/include/clang/Driver/Options.td
2865–2872

My impression is that generally for __builtin_XXX intrinsics, there are no compiler flags to make them available or remove their availability.
Is there a good reason why a command line option is needed for the __builtin_ptrauth intrinsics, but not (IIUC) for most or any other existing __builtin_XXX intrinsic?
If there is no good reason, it seems better to me to not have a command line option so there is better consistency across all __builtin_XXX intrinsics?

(after having read more of the patch): my impression has changed now that the f(no-)ptrauth-intrinsics flag rather selects whether the ptrauth intrinsics get lowered to PAuth hardware instructions, or to "regular" instructions emulating the behavior of authenticated pointers. If that is correct (and assuming it's a useful option to have), I would guess a different name for the command line option could be less misleading. As is, it suggests this selects whether ptrauth_ intrinsics are available or not. If instead, as I'm guessing above, this selects whether ptrauth_ intrinsics get lowered to PAuth instructions or not, maybe something like '-femulate-ptrauth' would describe the effect of the command line switch a bit better?

clang/lib/Headers/ptrauth.h
19–37

I think, but am not sure, that the decision of which keys are process independent and which ones are process-dependent is a software platform choice?
If so, maybe ptrauth_key_process_{in,}dependent_* should only get defined conditionally?
I'm not sure if any decisions have been taken already for e.g. linux, Android, other platforms.
If not, maybe this block of code should be surrounded by an ifdef that is enabled only when targeting Darwin?

tschuett added a comment.EditedNov 5 2021, 9:16 AM

If you look at the immintrin.h header, the access to many builtins is guarded by ifdefs.
#if defined(__SSSE3__)

The builtin __builtin_ia32_reduce_smin_d512 is useless on aarch64.

rjmccall added inline comments.Nov 5 2021, 11:02 PM
clang/include/clang/Driver/Options.td
2865–2872

The ptrauth features were implemented gradually, beginning with the intrinsics. Originally we needed a way to enable the intrinsics feature without relying on target information. We do still need a way to enable them without necessarily enabling automatic type/qualifier-based pointer authentication. I don't know if we need to be able to *disable* them when the target supports them; I agree that that would be a little strange.

If not, we could just enable the intrinsics whenever either the target says they're okay or software emulation (a separate, experimental feature) is enabled. The AArch64 target has a +pauth target feature. However, I don't know if -arch arm64e actually adds that feature on Apple targets. Also, the HasPAuth field in the clang TargetInfo does not appear to be properly initialized to false when +pauth *isn't* present; fortunately, that field never used.

I'm not sure if it would actually be okay to remove the -fptrauth-intrinsics driver option if we just enabled the intrinsics based on the target feature. That does feel cleaner, but unfortunately, we at Apple probably have explicit uses of the option that we'd have to clean up before we could remove the option. We could treat that as an Apple problem and keep it out of the open source tree, though, and maybe remove the option altogether someday.

Ahmed, thoughts?

clang/lib/Headers/ptrauth.h
19–37

Yes, in retrospect it was a bad idea to define these particular generic names. I believe Apple platforms no longer have "process-independent" keys. It should really just be (1) the concrete keys, (2) recommended default keys for code and data pointers, and then (3) the specific keys used in specific schemas. Beyond that, if people want a specific different key for some purpose, they should ask for it.

Unfortunately, there's already a fair amount of code using these names. We could deprecate the old names and point people towards the new names, though.

kristof.beyls added inline comments.Nov 8 2021, 1:20 AM
clang/lib/Headers/ptrauth.h
19–37

Thanks for those background insights!
I was thinking that maybe the keys that should be deprecated could be enabled only when targeting Apple platforms? I'm assuming here that most existing code using these only target Apple platforms; so making them available only when targeting Apple platforms could help with not letting the use of them spread further without impacting existing code much?

ab added inline comments.Nov 8 2021, 1:41 PM
clang/include/clang/Driver/Options.td
2865–2872

Hmm, I agree it would be strange to need to disable the intrinsics, but we do also gate the various higher-level qualifiers (and intrinsics) on ptrauth_intrinsics. So, in ptrauth.h (and in various users) the feature now really means "we're in a 'ptrauth-aware' environment". And it does make more sense to keep that separate from "we're running on a CPU that theoretically could support ptrauth". It comes down to what "ptrauth-aware" really means, and that's probably also an Apple problem, and all current users of ptrauth_intrinsics should use something like __arm64e__ instead.

That still means there's no equivalent for other targets and/or software emulation, but that seems okay: ptrauth.h already needs changes to be usable from anywhere other than arm64e (cf. the discussion about keys), and we can cross that bridge when we get there.

(One could argue that all the language-feature-specific qualifiers and intrinsics should be gated on the appropriate ptrauth_whatever feature, but the qualifiers are often used in precisely the glue/runtime code that doesn't build in the appropriate mode, so doesn't have the feature enabled.)

So, concretely, we could:

  • continue gating these plain intrinsics on ptrauth_intrinsics in ptrauth.h (IIRC there's an ACLE feature macro but it's specific to return address signing and BTI defaults; I'll check)
  • enable the feature when +pauth
  • replace all other uses of ptrauth_intrinsics with __arm64e__, in both ptrauth.h (gating the ABI qualifiers) and gradually in our internal codebases

and keep -fptrauth-intrinsics downstream for the transition (or, depending on how much the flag is really used, just keep the old behavior of enabling the feature for arm64e only; but yeah, that's my downstream problem)

clang/lib/Headers/ptrauth.h
19–37

Or we could keep these downstream as well, and deprecate them there directly.

Worth mentioning we'll have a similar problem with the "language-feature-specific" key enums and qualifiers (e.g., ptrauth_key_function_pointer): they're currently hardcoded in ptrauth.h with the arm64e values. I was thinking maybe they should be defined by the frontend and exposed to ptrauth.h through macros (and/or builtin types, or something else), so that the single source of truth is the frontend logic that decides which schemas are used where. But that's a problem for another day.

So, concretely:

  • remove these 4 key aliases (and transition them downstream, yadda yadda)
  • gate the later key aliases (that describe the language ABI schemas) on __arm64e__
  • figure out some more complicated way of defining these, as needed, for other targets
bruno added a comment.Nov 9 2021, 12:00 AM

Thanks for working on upstreaming this @ab. Overall looks good to me, I see clang-format issues, are those legit? One more comment inline.

clang/include/clang/Driver/Options.td
2865–2872

I'd vote for keeping as is: -fptrauth-intrinsics allows a nice limited use of the pauth feature pack. Has actually been useful for us (non-Apple targets) in code that needs signing capabilities but not want itself to be codegen'd using pauth - e.g. a dynamic linker for a system that is migrating codebase to pauth in small steps.

ptrauth_intrinsics gates have been equally useful. By doing this downstream we would need to duplicate this logic as well, so I don't really it benefiting the community as much.

Enabling the feature when +pauth sounds like overall goodness regardless imo.

kristof.beyls added inline comments.Nov 10 2021, 12:28 AM
clang/include/clang/Driver/Options.td
2865–2872

Thanks for sharing your experience @bruno .
I have to confess I do not fully understand "code that needs signing capabilities but not want itself to be codegen'd using pauth." I'm assuming this means "the ptrauth intrinsics must be available, but the compiler must not automatically insert pointer signing/authenticating instructions"?

If I understand that correctly, I'm still wondering if it's useful to have a command line switch that removes ptrauth intrinsics, rather than relying on ptrauth.h having the appropriate ifdefs to remove intrinsics when targeting something where they cannot work?

With the above, I guess "-f(no)ptrauth-intrinsics" then actually means "let the compiler automatically insert signing/authenticating instructions as defined by the default signing scheme for the target triple you're targeting"?

Maybe my confusion would be less if this patch also adds documentation for the command line switch.
I'm not sure where that documentation would best live. Maybe at https://clang.llvm.org/docs/UsersManual.html#command-line-options?

bruno added inline comments.Nov 18 2021, 6:26 PM
clang/include/clang/Driver/Options.td
2865–2872

I have to confess I do not fully understand "code that needs signing capabilities but not want itself to be codegen'd using pauth." I'm assuming this means "the ptrauth intrinsics must be available, but the compiler must not automatically insert pointer signing/authenticating instructions"?

Exactly, I'd like to see such instructions only being emitted via intrinsic usage.

If I understand that correctly, I'm still wondering if it's useful to have a command line switch that removes ptrauth intrinsics, rather than relying on ptrauth.h having the appropriate ifdefs to remove intrinsics when targeting something where they cannot work?

I think the -fnoptrauth-intrinsics is useful in practice, it provides quick workarounds when trying to untangle bugs in production, the "cancel previous added flag" behavior. I do not see a semantic meaning of its existence though.

With the above, I guess "-f(no)ptrauth-intrinsics" then actually means "let the compiler automatically insert signing/authenticating instructions as defined by the default signing scheme for the target triple you're targeting"?

We should also be able to use the intrinsics regardless of the target triple setup (and error out at driver time in case the instructions aren't supported by the backend).

The problem is that I might still need to disable automatically codegen of those instructions, but be able to use intrinsics to control their usage. Would there be another combination of driver flags to that effect?

Maybe my confusion would be less if this patch also adds documentation for the command line switch. I'm not sure where that documentation would best live. Maybe at https://clang.llvm.org/docs/UsersManual.html#command-line-options?

Agreed!