llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

[PAC][Driver] Implement `-mbranch-protection=pauthabi` option

Open kovdan01 opened this issue 1 year ago • 2 comments

Enable the following ptrauth flags when pauthabi is passed as branch protection:

  • intrinsics;
  • calls;
  • returns;
  • auth-traps;
  • vtable-pointer-address-discrimination;
  • vtable-pointer-type-discrimination;
  • init-fini.

Co-authored-by: Anatoly Trosinenko [email protected]

kovdan01 avatar Jun 30 '24 22:06 kovdan01

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Daniil Kovalev (kovdan01)

Changes

Enable the following ptrauth flags when pauthabi is passed as branch protection:

  • intrinsics;
  • calls;
  • returns;
  • auth-traps;
  • vtable-pointer-address-discrimination;
  • vtable-pointer-type-discrimination;
  • init-fini.

Co-authored-by: Anatoly Trosinenko <atrosinenko@accesssoftek.com>


Full diff: https://github.com/llvm/llvm-project/pull/97237.diff

4 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+38)
  • (modified) clang/test/Driver/aarch64-ptrauth.c (+28-8)
  • (modified) llvm/include/llvm/TargetParser/ARMTargetParserCommon.h (+1)
  • (modified) llvm/lib/TargetParser/ARMTargetParserCommon.cpp (+5-1)
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 1b7cc82ea816e..4ed1ece22b7aa 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1484,6 +1484,39 @@ void AddUnalignedAccessWarning(ArgStringList &CmdArgs) {
 }
 }
 
+static void handlePAuthABIOption(const ArgList &DriverArgs,
+                                 ArgStringList &CC1Args, const Driver &D) {
+  if (!DriverArgs.hasArg(options::OPT_fptrauth_intrinsics,
+                         options::OPT_fno_ptrauth_intrinsics))
+    CC1Args.push_back("-fptrauth-intrinsics");
+
+  if (!DriverArgs.hasArg(options::OPT_fptrauth_calls,
+                         options::OPT_fno_ptrauth_calls))
+    CC1Args.push_back("-fptrauth-calls");
+
+  if (!DriverArgs.hasArg(options::OPT_fptrauth_returns,
+                         options::OPT_fno_ptrauth_returns))
+    CC1Args.push_back("-fptrauth-returns");
+
+  if (!DriverArgs.hasArg(options::OPT_fptrauth_auth_traps,
+                         options::OPT_fno_ptrauth_auth_traps))
+    CC1Args.push_back("-fptrauth-auth-traps");
+
+  if (!DriverArgs.hasArg(
+          options::OPT_fptrauth_vtable_pointer_address_discrimination,
+          options::OPT_fno_ptrauth_vtable_pointer_address_discrimination))
+    CC1Args.push_back("-fptrauth-vtable-pointer-address-discrimination");
+
+  if (!DriverArgs.hasArg(
+          options::OPT_fptrauth_vtable_pointer_type_discrimination,
+          options::OPT_fno_ptrauth_vtable_pointer_type_discrimination))
+    CC1Args.push_back("-fptrauth-vtable-pointer-type-discrimination");
+
+  if (!DriverArgs.hasArg(options::OPT_fptrauth_init_fini,
+                         options::OPT_fno_ptrauth_init_fini))
+    CC1Args.push_back("-fptrauth-init-fini");
+}
+
 static void CollectARMPACBTIOptions(const ToolChain &TC, const ArgList &Args,
                                     ArgStringList &CmdArgs, bool isAArch64) {
   const Arg *A = isAArch64
@@ -1537,11 +1570,16 @@ static void CollectARMPACBTIOptions(const ToolChain &TC, const ArgList &Args,
     if (!isAArch64 && PBP.Key == "b_key")
       D.Diag(diag::warn_unsupported_branch_protection)
           << "b-key" << A->getAsString(Args);
+    if (!isAArch64 && PBP.HasPauthABI)
+      D.Diag(diag::warn_unsupported_branch_protection)
+          << "pauthabi" << A->getAsString(Args);
     Scope = PBP.Scope;
     Key = PBP.Key;
     BranchProtectionPAuthLR = PBP.BranchProtectionPAuthLR;
     IndirectBranches = PBP.BranchTargetEnforcement;
     GuardedControlStack = PBP.GuardedControlStack;
+    if (isAArch64 && PBP.HasPauthABI)
+      handlePAuthABIOption(Args, CmdArgs, D);
   }
 
   CmdArgs.push_back(
diff --git a/clang/test/Driver/aarch64-ptrauth.c b/clang/test/Driver/aarch64-ptrauth.c
index fa0125f4b22a9..dc63545a47a86 100644
--- a/clang/test/Driver/aarch64-ptrauth.c
+++ b/clang/test/Driver/aarch64-ptrauth.c
@@ -13,13 +13,33 @@
 // RUN:   %s 2>&1 | FileCheck %s --check-prefix=ALL
 // ALL: "-cc1"{{.*}} "-fptrauth-intrinsics" "-fptrauth-calls" "-fptrauth-returns" "-fptrauth-auth-traps" "-fptrauth-vtable-pointer-address-discrimination" "-fptrauth-vtable-pointer-type-discrimination" "-fptrauth-init-fini"
 
+// RUN: %clang -### -c --target=aarch64 -mbranch-protection=pauthabi %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=PAUTHABI1
+// PAUTHABI1: "-cc1"{{.*}} "-fptrauth-intrinsics" "-fptrauth-calls" "-fptrauth-returns" "-fptrauth-auth-traps" "-fptrauth-vtable-pointer-address-discrimination" "-fptrauth-vtable-pointer-type-discrimination" "-fptrauth-init-fini"
+
+// RUN: %clang -### -c --target=aarch64 -mbranch-protection=pauthabi -fno-ptrauth-intrinsics \
+// RUN:   -fno-ptrauth-calls -fno-ptrauth-returns -fno-ptrauth-auth-traps \
+// RUN:   -fno-ptrauth-vtable-pointer-address-discrimination -fno-ptrauth-vtable-pointer-type-discrimination \
+// RUN:   -fno-ptrauth-init-fini %s 2>&1 | FileCheck %s --check-prefix=PAUTHABI2
+// PAUTHABI2-NOT: "-fptrauth-intrinsics"
+// PAUTHABI2-NOT: "-fptrauth-calls"
+// PAUTHABI2-NOT: "-fptrauth-returns"
+// PAUTHABI2-NOT: "-fptrauth-auth-traps"
+// PAUTHABI2-NOT: "-fptrauth-vtable-pointer-address-discrimination"
+// PAUTHABI2-NOT: "-fptrauth-vtable-pointer-type-discrimination"
+// PAUTHABI2-NOT: "-fptrauth-init-fini"
+
 // RUN: not %clang -### -c --target=x86_64 -fptrauth-intrinsics -fptrauth-calls -fptrauth-returns -fptrauth-auth-traps \
 // RUN:   -fptrauth-vtable-pointer-address-discrimination -fptrauth-vtable-pointer-type-discrimination \
-// RUN:   -fptrauth-init-fini %s 2>&1 | FileCheck %s --check-prefix=ERR
-// ERR:      error: unsupported option '-fptrauth-intrinsics' for target '{{.*}}'
-// ERR-NEXT: error: unsupported option '-fptrauth-calls' for target '{{.*}}'
-// ERR-NEXT: error: unsupported option '-fptrauth-returns' for target '{{.*}}'
-// ERR-NEXT: error: unsupported option '-fptrauth-auth-traps' for target '{{.*}}'
-// ERR-NEXT: error: unsupported option '-fptrauth-vtable-pointer-address-discrimination' for target '{{.*}}'
-// ERR-NEXT: error: unsupported option '-fptrauth-vtable-pointer-type-discrimination' for target '{{.*}}'
-// ERR-NEXT: error: unsupported option '-fptrauth-init-fini' for target '{{.*}}'
+// RUN:   -fptrauth-init-fini %s 2>&1 | FileCheck %s --check-prefix=ERR1
+// ERR1:      error: unsupported option '-fptrauth-intrinsics' for target '{{.*}}'
+// ERR1-NEXT: error: unsupported option '-fptrauth-calls' for target '{{.*}}'
+// ERR1-NEXT: error: unsupported option '-fptrauth-returns' for target '{{.*}}'
+// ERR1-NEXT: error: unsupported option '-fptrauth-auth-traps' for target '{{.*}}'
+// ERR1-NEXT: error: unsupported option '-fptrauth-vtable-pointer-address-discrimination' for target '{{.*}}'
+// ERR1-NEXT: error: unsupported option '-fptrauth-vtable-pointer-type-discrimination' for target '{{.*}}'
+// ERR1-NEXT: error: unsupported option '-fptrauth-init-fini' for target '{{.*}}'
+
+// RUN: not %clang -### -c --target=x86_64 -mbranch-protection=pauthabi %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=ERR2
+// ERR2: error: unsupported option '-mbranch-protection=' for target 'x86_64'
diff --git a/llvm/include/llvm/TargetParser/ARMTargetParserCommon.h b/llvm/include/llvm/TargetParser/ARMTargetParserCommon.h
index f6115718e9f5f..ca634ed969d84 100644
--- a/llvm/include/llvm/TargetParser/ARMTargetParserCommon.h
+++ b/llvm/include/llvm/TargetParser/ARMTargetParserCommon.h
@@ -43,6 +43,7 @@ struct ParsedBranchProtection {
   bool BranchTargetEnforcement;
   bool BranchProtectionPAuthLR;
   bool GuardedControlStack;
+  bool HasPauthABI;
 };
 
 bool parseBranchProtection(StringRef Spec, ParsedBranchProtection &PBP,
diff --git a/llvm/lib/TargetParser/ARMTargetParserCommon.cpp b/llvm/lib/TargetParser/ARMTargetParserCommon.cpp
index d6ce6581bb1a9..0b1e6d3356f68 100644
--- a/llvm/lib/TargetParser/ARMTargetParserCommon.cpp
+++ b/llvm/lib/TargetParser/ARMTargetParserCommon.cpp
@@ -140,7 +140,7 @@ ARM::EndianKind ARM::parseArchEndian(StringRef Arch) {
 // an erroneous part of the spec.
 bool ARM::parseBranchProtection(StringRef Spec, ParsedBranchProtection &PBP,
                                 StringRef &Err, bool EnablePAuthLR) {
-  PBP = {"none", "a_key", false, false, false};
+  PBP = {"none", "a_key", false, false, false, false};
   if (Spec == "none")
     return true; // defaults are ok
 
@@ -160,6 +160,10 @@ bool ARM::parseBranchProtection(StringRef Spec, ParsedBranchProtection &PBP,
       PBP.BranchTargetEnforcement = true;
       continue;
     }
+    if (Opt == "pauthabi") {
+      PBP.HasPauthABI = true;
+      continue;
+    }
     if (Opt == "pac-ret") {
       PBP.Scope = "non-leaf";
       for (; I + 1 != E; ++I) {

llvmbot avatar Jul 01 '24 17:07 llvmbot

Is there any thought on how we want to manage signing schemas going forward? For example I can imagine looking an environment from the triple to select a signing schema for a particular platform. I could also see a potential for a separate command line option to choose from documented named signing schemas.

I think we discussed this a bit and the conclusion was that it would be up to the platform to chose the appropriate signing scheme (and do ABI versioning if desired / necessary). However, currently there is no way to ask platform for this and it looks like a chicken-and-egg problem. So we may want to come with some "default" values that are more or less "good enough". These in the future might be fully overridden by a platform or we may chose alternative approach. For now it is just a way to combine different -fpauth-* options as it seems to me.

asl avatar Jul 02 '24 20:07 asl

Apologies for the delay, I was out of office Thursday/Friday and wanted to discuss some possible options on the Monday call. I'll write something up based on the discussion tomorrow (Tuesday 9th) morning. I am at a conference for a lot of the week, so I maybe a bit slower to respond.

smithp35 avatar Jul 08 '24 18:07 smithp35

Apologies, it looks like I'm not going to get this out this morning, still working on it.

smithp35 avatar Jul 09 '24 10:07 smithp35

Apologies for the length of the post, it could probably do with more revisions and research but I thought it better to send what I have and refine later after comments. Most of this is a summation of a discussion had in the PAuthABI call, followed by my attempts at analysing the options we discussed. My apologies if I've misrepresented or missed out anything in the call. Please feel free to correct me where I'm wrong or have missed something out. I'll be at a conference this week so I may be slow to respond.

Known use cases and background.

We have three known use cases for PAuthABI, with two in active development:

  • A platform either ELF or MachO based deploys PAuthABI in all or, most likely, a subset of programs.
  • A bare-metal system that can statically compile everything the same way for the device. This not in active development.
  • Enabling PAuthABI for testing in upstream LLVM.

On each of the platforms, there is expected to be more than onesigning schema, for example kernel vs user-space. With the possible exception of return address signing, each signing schema is its own ABI. While low-level options may exist to control the signing schema, we do not expect end users to use them as they risk breaking the platform ABI.

This infers that we need an option to choose between high-level signing schemas, that map to some combination of the low-level options. This mapping will need to be per-platform as Linux may choose differently to BSD.

The signing schema will also need to be used as the multilib key so that a compatible set of libraries can be linked for the signing schema.

On ELF the (platform, signing-schema) choice can be recorded in the ELF properties.

Users of the low-level command line options cannot be easily recorded in the ELF properties. There are various platform specific choices that could be made if the low-level options are used. For example:

  • Record the high-level signing schema and trust the user has written the code such that it is compatible.
  • Reject the low-level option.
  • Ignore the low-level option.
  • Record a custom signing schema.

I think that this has to be under control of the platform. My intuition is that we trust the user, perhaps with a warning, and record the high-level signing schema.

On a bare-metal embedded system each toolchain can make up their own signing schema, however no toolchain is likely to provide pre-compiled libraries for every possible choice so we expect there to be a sensible default signing schema.

Thoughts on -mbranch-protection=pauthabi

While pauthabi is a form of CFI like the other mbranch-protection options there are a number of differences that cause problems:

  • All existing -mbranch-protection options have a hint space implementation so are compatible with any architecture. PAuthABI requires an extension.
  • All existing -mbranch-protection options are compatible, pauthabi conflicts with standard and pac-ret.
  • All existing -mbranch-protection options are ABI neutral, pauthabi is not and any use of it on a standard linux platform would not work.
  • Not got an easy way to extend with the signing schema.

Alternatives discussed

A single option using -mabi:

Use the existing -mabi option to indicate the signing schema (or absence of signing schema) if pauthabi is not used. The -mabi option for AArch64 is currently permits aapcs (default hard-float) aapcs-soft and darwinpcs.

-mabi=pauth // default signing schema for the platform (OS in triple). -mabi=pauth<signing-schema> // named signing schema, interpreted per platform.

The expands to a number of low-level options. This is determined per platform. With this information we can derive a <Platform, signing-schema> for the ELF marking.

For example:

  • --target=aarch64-none-elf -mabi=pauth
  • --target=aarch64-linux-gnu -mabi=pauthkernel

There is an open question over what we should do with the theoretical combination of soft-floating point and pauth as these both use the -mabi=<abi> option. While each combination is its own ABI, the choice of float-abi is orthogonal to signing schema. I'm leaning towards suggesting a + or , separated list for -mabi with a default of aapcs (hard-float) so we don't end up doubling the number of options. So we would have -mabi=pauth<signing-schema>,aapcs-soft rather than -mabi=aapcs-soft-pauth<signing-schema>. I don't think that would need to be implemented until there is an actual need for it.

Two options using -mabi and -msigning-schema:

This is functionally equivalent to the single -mabi=pauth<signing-schema> split into two options.

  • -mabi=pauth enables pauthabi
  • -msigning-schema=<signing-schema> if -msigning-schema is absent then the default signing schema for the platform is used.

This has the advantage of being easier to parse, but has a couple of drawbacks:

  • Each -msigning-schema is a different ABI so it is logical to have a single -mabi option with disjoint choices.
  • -msigning-schema without -mabi=pauth is meaningless.

Use the environment part of the triple:

The ARM target uses the environment part of the triple to record the float abi, arm-linux-gnueabi for soft-float and arm-linux-gnueabihf for hard-float. With -mfloat-abi being normalised into the environment.

The signing schema could be added to the environment part of the triple, for example aarch64-linux-gnupauth or aarch64-linux-gnupauth<signing-schema>. The -mabi=pauth<signing-schema> or the alternative -mabi=pauth -msigning-schema=<signing-schema> are normalised into the triple.

One disadvantage of doing this is that we can't use a - in the name of a signing schema as it would affect the triple parsing.

Recommendation:

Right now we only have a single signing schema used for upstream LLVM testing on the Linux target. We expect the first concrete use in downstrean platforms. We don't need to add support upstream for multiple signing schemas until they are needed. We would therefore only need to implement a default signing schema for -mabi=pauth

We could choose to implement -msigning-schema or -mabi=pauth<signing-schema> at a later time.

I don't have enough experience with triples to know whether encoding the information in the triple is useful or not. I think it is important to have options outside of the triple that a possible implementation in GCC could use, but whether these are normalised into the triple or not doesn't seem to be a problem.

I think we should require the +pauth feature (default in armv8.3-A) for -mabi=pauth<signing-schema> to be accepted.

I think the combinations of -mbranch-protection that are not compatible with pauth should be errors, or ignored with a warning.

Open question:

If we are being as careful to avoid prematurly fixing a signing schema for the linux target we could use a of test and no -mabi=pauth default. For example -mabi=pauthtest.

I think we could use -mabi=pauth if we mark the feature as experimental or alpha, this default signing schema may change until a future release when experimental/alpha is removed.

smithp35 avatar Jul 09 '24 22:07 smithp35

Thanks @smithp35 for a detailed description!

Use the environment part of the triple

I suppose we should use this this option since it's harder for end users to use that incorrectly - it looks like that many real-world code and corresponding build environments rely on triple and do not encounter other options (for example, as you said, hard/soft float are encoded in triple or normalized to triple if passed separately).

I'll implement initial support for that and update current PR.

I think we should require the +pauth feature (default in armv8.3-A) for -mabi=pauth<signing-schema> to be accepted.

We actually have a similar issue already opened, see #97332, so yes, it's a known feature request.

If we are being as careful to avoid prematurly fixing a signing schema for the linux target we could use a of test and no -mabi=pauth default. For example -mabi=pauthtest.

I like this idea. Of course, we can mark functionality as "alpha" or "experimental", but just using a different option value makes this more explicit and end users will not have a chance to mix "alpha" stuff with "released" stuff when we have one.

I think that this has to be under control of the platform. My intuition is that we trust the user, perhaps with a warning, and record the high-level signing schema.

In general, I agree with that point, but for the first "adoption" phase (while we have -mabi=pauthtest or -mabi=pauth marked as "alpha") it might be useful to encode the low-level schema - we let users compile with -fptrauth-* flags if they want (probably with a huge warning that they are using this at their own risk), but we do not let them link incompatible binaries. At least, I suggest to limit scope of the PR to implementing -mabi=pauth (normalizing to triple), parsing the triple and enabling proper features correspondingly, while leaving low-level encoding of signing schema in pauth core info (inside elf's gnu property) already implemented untouched as for now.

kovdan01 avatar Jul 10 '24 08:07 kovdan01

@smithp35 I've implemented pauthtest ABI support - see new PR description for details and tests for examples. I'm not sure if the implementation is nice and maybe I've put some logic in wrong places while there are better ones, but the result looks matching the requirements discussed above. One of the thoughts I have is that maybe we should implement a new option like -mpauth-abi= like we already have -mfloat-abi for arm. In -mabi, we can pass things like aapcs, and I'm not sure if PAuth stuff should be messed with procedure calls standard.

Please let me know your thoughts on the implementation.

Anyone else interested is also welcome to leave feedback. Tagging @MaskRay

kovdan01 avatar Jul 12 '24 21:07 kovdan01

Thanks @smithp35

I opened https://github.com/llvm/llvm-project/issues/99950 to track this

asl avatar Jul 22 '24 21:07 asl