Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PAC][Driver] Support pauthtest ABI for AArch64 Linux triples #97237

Merged
merged 16 commits into from
Jul 22, 2024

Conversation

kovdan01
Copy link
Contributor

@kovdan01 kovdan01 commented Jun 30, 2024

When pauthtest is either passed as environment part of AArch64 Linux triple
or passed via -mabi=, enable the following ptrauth flags:

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

Some related stuff is still subject to change, and the ABI itself might
be changed, so end users are not expected to use this and the ABI name
has 'test' suffix.

If -mabi=pauthtest option is used, it's normalized to effective triple.

When the environment part of the effective triple is pauthtest, try
to use aarch64-linux-pauthtest as multilib directory.

The following is not supported:

  • combination of pauthtest ABI with any branch protection scheme except BTI;
  • explicit set of environment part of the triple to a value different
    from pauthtest in combination with -mabi=pauthtest;
  • usage on non-Linux OS.

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>
@kovdan01 kovdan01 self-assigned this Jun 30, 2024
@kovdan01 kovdan01 requested review from asl and atrosinenko June 30, 2024 22:02
@kovdan01 kovdan01 added this to the LLVM 19.X Release milestone Jun 30, 2024
@kovdan01 kovdan01 linked an issue Jun 30, 2024 that may be closed by this pull request
@kovdan01 kovdan01 marked this pull request as ready for review July 1, 2024 17:06
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jul 1, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 1, 2024

@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) {

@@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See addOptInFlag.

But the implementation seems quite different from the title/description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See addOptInFlag.

Do you mean that we need to replace

  if (!DriverArgs.hasArg(options::OPT_fptrauth_xxx,
                         options::OPT_fno_ptrauth_xxx))
    CC1Args.push_back("-fptrauth-xxx");

with

  DriverArgs.addOptInFlag(CC1Args, options::OPT_fptrauth_xxx,
                          options::OPT_fno_ptrauth_xxx);

...?

If so, this does not look correct - addOptInFlag would add the flag present (if any) in DriverArgs to CC1Args, but we want to append a list of ptrauth flags to cc1 args unconditionally if pauthabi is passed as branch protection.

Do I miss smth? I suppose I might have misunderstood you point.

But the implementation seems quite different from the title/description.

Hmm, it actually looks consistent to me, the implementation seems matching the description from my point of view - we want to add a bunch of arguments with -mbranch-protection=pauthabi used as a shortcut, we do that. Could you please describe what is inconsistent between description and implementation in a bit more detail?

Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it we are implicitly defining the default signing schema for ELF platforms with -mbranch-protection=pauthabi .

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 don't think these need to implemented now, would be good to make sure that there are enough comments stating the ABI implications of making changes.

@@ -1484,6 +1484,39 @@ void AddUnalignedAccessWarning(ArgStringList &CmdArgs) {
}
}

static void handlePAuthABIOption(const ArgList &DriverArgs,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be worth a comment above and possibly inline explaining the ABI implications of the options.

For example:

Each combination of options here forms a signing schema, and in most cases each signing schema is its own incompatible ABI. The default values of the options represent the default signing schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the comment, thanks! See fcd090c

Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it is worth resurrecting https://discourse.llvm.org/t/aarch64-pauthabi-options-for-command-line-options-to-use-the-pauthabi-and-set-signing-schema/73479 to see if we can get some more visibility on this. It has also been almost a year since that was posted.

Some colleagues expressed some concern that as this was introducing a new ABI, while the existing -mbranch-protection options are ABI neutral [*], then perhaps a better option would be to use the -mabi option. For example -mabi=pauthabi.

There are some advantages and disadvantages to doing that. It is explicit that there is a different ABI being used, however it is logically an extension of the CFI features in -mbranch-protection and some of the branch-protection modifiers such as +ret could be useful.

Will be worth having some thoughts on how you would want to interact with existing mbranch-protection options. If pauthabi would supersede all the pointer authentication parts then it may be better to have a -mabi option?

[*] Stack unwinding needs to be aware of signed return addresses, but these can be optional.

Scope = PBP.Scope;
Key = PBP.Key;
BranchProtectionPAuthLR = PBP.BranchProtectionPAuthLR;
IndirectBranches = PBP.BranchTargetEnforcement;
GuardedControlStack = PBP.GuardedControlStack;
if (isAArch64 && PBP.HasPauthABI)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need any additional error checking for the existing branch protection options that affect pointer authentication?

For example we have

pac-ret // return address signing with A key
pac-ret+leaf // extend return address signing to leaf functions
standard = pac-ret+bti+pc // enable pac-ret, bti and pc (if available on hardware).
pc // Enable pc as modifier in return address signing.
b-key // Use b-key for signing return address.

When pauthabi is used, are the other PAC related options ignored? I can see +leaf being potentially useful, as well as +pc. I think b-key is going to clash with the signing schema.

The other options look to be subsets of pauthabi (unless additional command line options unless -fno-ptrauth-returns is used.

Copy link
Contributor Author

@kovdan01 kovdan01 Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest not to support pauthabi in combination with other branch protection options as for now. Here are the reasons why.

  1. pac-ret: this and -fptrauth-returns (which is enabled by -mbranch-protection=pauthabi) are intended to do similar stuff, but the implementation differs.

    For -mbranch-protection=pac-ret:

    • sign-return-address llvm module flag is set (and optionally sign-return-address-all with +leaf);
    • PAUTH_PROLOGUE and PAUTH_EPILOGUE pseudo-instructions are emitted in AArch64FrameLowering::emitPrologue and AArch64FrameLowering::emitEpilogue; these pseudos are later expanded by AArch64PointerAuth pass;
    • both A and B keys can be used (depending on +b-key and corresponding function attribute sign-return-address-key or module flag sign-return-address-with-bkey);
    • using pc register as additional modifier is supported with +pc (corresponding module flag is branch-protection-pauth-lr).

    For -fptrauth-returns (I'll talk about downstream Apple implementation since many parts are not upstreamed yet, see, for example, swiftlang@13f9944):

    • ptrauth-returns attribute is set on functions we want this to be enabled;
    • actual codegen logic is implemented in AArch64FrameLowering::emitPrologue and AArch64FrameLowering::emitEpilogue - we emit actual instructions like pacibsp directly there;
    • B key is always used;
    • using pc register as additional modifier is not supported.

    If we try to enable both by -mbranch-protection=pauthabi+pac-ret, it'll result in incorrect code with duplicating sign/auth instructions. For example, for this:

    int a() {
      return b() + c();
    }
    

    We get this:

    a:
      paciasp
      pacibsp
      stp     x29, x30, [sp, #-32]!           // 16-byte Folded Spill
      str     x19, [sp, #16]                  // 8-byte Folded Spill
      mov     x29, sp
      bl      b
      mov     w19, w0
      bl      c
      add     w0, w0, w19
      ldr     x19, [sp, #16]                  // 8-byte Folded Reload
      ldp     x29, x30, [sp], #32             // 16-byte Folded Reload
      autiasp
      retab
    

    A corresponding issue was already previously opened (mistakenly in mainline llvm repo while it was and actually still is an issue specific for Apple downstream). Links:

    I'll probably re-open the issue in mainline repo when codegen support for ptrauth-returns is upstreamed. Alternatively, the Apple's downstream implementation for return address signing might be dropped since pac-ret seems to be more complete, and we can use -fptrauth-returns for setting the same return address signing options as pac-ret+b-key. Tagging @ahmedbougacha.

  2. +leaf and +pc: these are only allowed with pac-ret, and while it's not clear how we'll resolve collisions between pac-ret and ptrauth-returns (which is part of pauthabi), it's probably better to just disallow pauthabi+leaf and pauthabi+pc.

  3. +b-key: ptrauth-returns (which is part of pauthabi) already uses B key by default (but the codegen support is still present only in Apple downstream, see swiftlang@13f9944)

  4. gcs: as far as I understand, guarded control stack is smth like what is usually called shadow stack. I'm not sure how it's supposed to work with return address signing - probably, these shouldn't be used together, so, since pauthabi implies return address signing, disallow pauthabi+gcs.

  5. bti: depending on operand value (c, j or jc), the bti instruction inserted at beginning of valid call/jump targets checks that PSTATE.BTYPE matches the value set by blr and/or br instructions (see https://developer.arm.com/documentation/100076/0100/A64-Instruction-Set-Reference/A64-General-Instructions/BTI). As far as I understand, instructions like blraa do not set this state (at least, I was unable to find info regarding this), so further bti instruction will fail. @smithp35 Could you please clarify if blraa and other authenticating branch instruction set PSTATE.BTYPE which bti instructions treat as correct?

  6. standard - a combination of bti, gcs, pac-ret w/o +leaf and, optionally, +pc - so, shouldn't be supported with pauthabi since its parts are not supported.

Regarding disallowing separate flags like -fptrauth-returns with other branch protection schemes - I don't think we need to do that since the -fptrauth-* flags are not intended to be used directly - instead, we propose this "umbrella" option -mbranch-protection=pauthabi, which enabled a "good enough" set of flags at once.

Please let me know your thought about that. I'll update the PR with corresponding checks as soon as we come to some consensus on the question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - I'm going to change the sign-return-address and variants to be a function flag. (#82819)

bti - All BRA* and BLRA* instructions set the PSTATE.BTYPE. I prefer to check the pseudo codefor this things (see BTypeNext)

IMHO pauthabi+bti makes sense as they are complementary while other option are overlapping. We can introduce this combination later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielKristofKiss Thanks! Yes, I now see that authenticating branch instructions also set the PSTATE.BTYPE. So, I agree that we can allow pauthabi+bti. The codegen when pauthabi+bti is passed as branch protection looks OK on downstream:

In mainline, codegen with -fptrauth-returns is not implemented yet, so I can't provide tests verifying generated assembly correctness right now. I'll add them as soon as codegen support for this flag is merged.

I've implemented a check which only allows bti with pauthabi and does not allow pac-ret[+leaf,+pc,+b-key] and gcs. See 3067c93

Tagging @smithp35

@asl
Copy link
Collaborator

asl commented Jul 2, 2024

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.

@kovdan01 kovdan01 requested review from smithp35 and MaskRay July 4, 2024 22:30
// 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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One -NOT: "-fptrauth should catch all undesired "-fptrauth-*" options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed, thanks, see 1d81b91

@kovdan01 kovdan01 requested a review from MaskRay July 4, 2024 23:15
@smithp35
Copy link
Collaborator

smithp35 commented Jul 9, 2024

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

@smithp35
Copy link
Collaborator

smithp35 commented Jul 9, 2024

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.

@kovdan01
Copy link
Contributor Author

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 added 2 commits July 13, 2024 00:23
When `pauthtest` is either passed as environment part of AArch64 Linux triple
or passed via `-mabi=`, enable the following ptrauth flags:

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

Some related stuff is still subject to change, and the ABI itself might
be changed, so end users are not expected to use this and the ABI name
has 'test' suffix.

If `-mabi=pauthtest` option is used, it's normalized to effective triple.

When the environment part of the effective triple is `pauthtest`, try
to use `aarch64-linux-pauthtest` as multilib directory.

The following is not supported:
- combination of `pauthtest` ABI with any branch protection scheme except BTI;
- explicit set of environment part of the triple to a value different
  from `pauthtest` in combination with `-mabi=pauthtest`;
- usage on non-Linux OS.
@kovdan01 kovdan01 force-pushed the branch-protection-pauthabi branch from 0ac4724 to f06bc88 Compare July 12, 2024 21:23
@kovdan01 kovdan01 changed the title [PAC][Driver] Implement -mbranch-protection=pauthabi option [PAC][Driver] Support pauthtest ABI for AArch64 Linux triples Jul 12, 2024
@kovdan01 kovdan01 closed this Jul 12, 2024
@kovdan01 kovdan01 deleted the branch-protection-pauthabi branch July 12, 2024 21:25
@kovdan01 kovdan01 restored the branch-protection-pauthabi branch July 12, 2024 21:25
@kovdan01 kovdan01 reopened this Jul 12, 2024
@kovdan01
Copy link
Contributor Author

@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

@@ -324,6 +324,9 @@ ARMTargetInfo::ARMTargetInfo(const llvm::Triple &Triple,
case llvm::Triple::GNU:
setABI("apcs-gnu");
break;
case llvm::Triple::PAuthTest:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to touch 32-bit ARM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we do not need that, thanks for bringing attention to this.

Deleted here and in ARM::computeDefaultTargetABI, see eae6f7c

@kovdan01 kovdan01 requested a review from asl July 16, 2024 19:38
Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay in responding, been a bit backed up. I like the idea of pauthtest as it does give some leeway to change the signing schema.

I expect that with some work this could be made to work with bare-metal targets too, but I think it is best to stick with what has been tested so far.

Not had a chance to go through the tests in detail yet. Will aim to do that before the end of the week.

if (IndirectBranches)
CmdArgs.push_back("-mbranch-target-enforce");
if (GuardedControlStack)
if (GuardedControlStack) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think GuardedControlStack should be compatible with PAuthABI but there won't be hardware available for some time so we could retrospectively support it later after testing it.

Perhaps worth a comment that GCS is untested with PAuthABI, but could be enabled after testing with a suitable system.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment, thanks! See 4f3da9e

@kovdan01 kovdan01 requested a review from smithp35 July 18, 2024 08:50
@@ -0,0 +1,4 @@
// RUN: %clang --target=aarch64-linux-pauthtest --sysroot=%S/Inputs/multilib_aarch64_linux_tree -### -c %s 2>&1 | FileCheck %s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks this is not used: "clang/test/Driver/Inputs/multilib_aarch64_linux_tree/usr/include/aarch64-linux-gnu/.keep"
BTW we could just add to the test:
rm -rf %t.dir && mkdir -p %t.dir/multilib_aarch64_linux_tree/usr/include/aarch64-linux-gnu/ && mkdir -p %t.dir/multilib_aarch64_linux_tree/usr/include/aarch64-linux-pauthtest/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks this is not used: "clang/test/Driver/Inputs/multilib_aarch64_linux_tree/usr/include/aarch64-linux-gnu/.keep"

It's not directly used in this test, but I've added that in order to have a choice between two directories (one with default triple, one with pauthtest) and check that the pauthtest directory is chosen. Is this considered unneeded?

BTW we could just add to the test

Yes, that's possible, thanks for suggestion! But I've followed convention with already present multilib_arm_linux_tree (see clang/test/Driver/arm-multilibs.c test). I'm not sure what's better - stick with old existing conventions or not clutter source tree with empty directories which can be created on demand during test. So, I'll leave this "as is" currently and change this if there are more requests.

If you actually have a strong preference for not keeping empty multilib folders in source tree and just creating directories during test - please let me know.

Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion in the PAuthABI call. We agreed that it would be best to have an exemplar of how a signing schema for a platform should be encoded rather than always using the individual options.

Anton mentioned that we can document that pauthtest is not a stable signing schema to be used in production, and if there ended up being a stable upstream linux signing schema -mabi=pauth (probably) then pauthtest could be removed.

@kovdan01 kovdan01 merged commit 146fd7c into llvm:main Jul 22, 2024
7 checks passed
@asl asl mentioned this pull request Jul 22, 2024
@asl
Copy link
Collaborator

asl commented Jul 22, 2024

Thanks @smithp35

I opened #99950 to track this

yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
When `pauthtest` is either passed as environment part of AArch64 Linux
triple
or passed via `-mabi=`, enable the following ptrauth flags:

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

Some related stuff is still subject to change, and the ABI itself might
be changed, so end users are not expected to use this and the ABI name
has 'test' suffix.

If `-mabi=pauthtest` option is used, it's normalized to effective
triple.

When the environment part of the effective triple is `pauthtest`, try
to use `aarch64-linux-pauthtest` as multilib directory.

The following is not supported:
- combination of `pauthtest` ABI with any branch protection scheme
except BTI;
- explicit set of environment part of the triple to a value different
  from `pauthtest` in combination with `-mabi=pauthtest`;
- usage on non-Linux OS.

---------

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

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[PAC] Implement -mbranch-protection=pauthabi
6 participants