[AArch64] Return Address Signing B Key Support

[LukeCheeseman]

Summary

• Add command line options for selecting the B key when enabling return address signing (also tidy up the msign_return_address tablegen definiton by suffixing the option with EQ)
• This expands the -msign-return-address=<scope> to now be -msign-return-address=<scope>[+<key>] where <key> is one of a_key or b_key. The default key is a_key.
• Generate the "sign-return-address-key" function attribute with the relevant key

[SjoerdMeijer]

test/CodeGen/aarch64-sign-return-address.c

3–5

If the default is the a_key, does this test need to check the a_key attribute?

[olista01]

This looks like it has the same problem as D51418 (doesn't get applied to C++ static constructor functions).

[LukeCheeseman]

• updated tests to check the default a_key attribute is included in functions that have the return address signing scope attribute
• setting the return-address-sign-key attribute of global static constructors

[LukeCheeseman]

• Introduce the -mbranch-protection option. This is to be used for both pointer authentication and branch protection security features.
  • The branch protection compiler support will follow later
• The options available are -mbranch-protection=<type>(+<type>)* where <type> ::= [bti,pac-ret[+leaf,b-key]*]
• This should be the primary way of using branch protection features and the -msign-return-address option should be deprecated
• Remove the b-key selection from the earlier patch for -msign-return-address

[javed.absar]

include/clang/Frontend/CodeGenOptions.h

117

Perhaps a line of comment on each enum-value would be useful (much like the code around).

[LukeCheeseman]

include/clang/Frontend/CodeGenOptions.h

117

I'm not sure what value there would be in adding extra comments here, I think the values are fairly self explanatory.

[olista01]

lib/Frontend/CompilerInvocation.cpp

1133

The driver code is emitting a separate -msign-return-address-key= option, but this is expecting the key to still be part of -msign-return-address=, so we can never end up selecting the B key. Out of the two approaches, I prefer having multiple simpler options in CC1, so that we don't have to repeat all of the parsing that happens in the driver.

Also, you're emitting the -mbranch-target-enforce option in the driver, but not handling it here. We should either reject it in the driver for now, or add the CC1 part of that in this patch.

[LukeCheeseman]

• Stop parsing msign-return-address as a scope and key pair
• pass bti value through to the CC1 driver and handle it, this adds the branch-target-enforce attribute to functions

[olista01]

lib/CodeGen/CGDeclCXX.cpp

364

LLVM style has the opening brace on the same line as the if. There's a git-clang-format script in the clang repo which can fix up style issues like this for a whole patch, changing touching lines you haven't changes.

374

I think we need to handle branch-target-enforcement here too.

lib/CodeGen/TargetInfo.cpp

4988

Style nit: brace on same line as if.

lib/Driver/ToolChains/Clang.cpp

1426

Please add a comment about what the parts of the return value are.

1433

I'm not sure what this is trying to say. Is it referring to the "standard" case below?

1441

I'd make this 4, because that's the longest sensible argument ("pac-ret+leaf+b-key+bti").

1453

Add a comment about the fact that "leaf" and "b-key" must follow "pac-ret", to explain why we don't just parse this in one loop.

1483

Nit: brace on same line as if.

test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp

11

This should also test branch target enforcement (it's also missing from the code).

[olista01]

LGTM, thanks!

[LukeCheeseman]

This was committed under 345273. (Forgot to mention the revision in the commit message)

include/clang/Driver/Options.td

def mfix_cortex_a53_835769 : Flag<["-"], "mfix-cortex-a53-835769">,
   HelpText<"Workaround Cortex-A53 erratum 835769 (AArch64 only)">;
 def mno_fix_cortex_a53_835769 : Flag<["-"], "mno-fix-cortex-a53-835769">,
   Group<m_aarch64_Features_Group>,
   HelpText<"Don't workaround Cortex-A53 erratum 835769 (AArch64 only)">;
 foreach i = {1-7,18,20} in
   def ffixed_x#i : Flag<["-"], "ffixed-x"#i>, Group<m_aarch64_Features_Group>,
     HelpText<"Reserve the "#i#" register (AArch64 only)">;
 
-def msign_return_address : Joined<["-"], "msign-return-address=">,
-  Flags<[CC1Option]>, Group<m_Group>,
-  HelpText<"Select return address signing scope">, Values<"none,all,non-leaf">;
+def msign_return_address_EQ : Joined<["-"], "msign-return-address=">,
+  Flags<[CC1Option]>, Group<m_Group>, Values<"none,all,non-leaf[+a_key,b_key]">,
+  HelpText<"Select return address signing scope">;
 
 def msimd128 : Flag<["-"], "msimd128">, Group<m_wasm_Features_Group>;
 def mno_simd128 : Flag<["-"], "mno-simd128">, Group<m_wasm_Features_Group>;
 def mnontrapping_fptoint : Flag<["-"], "mnontrapping-fptoint">, Group<m_wasm_Features_Group>;
 def mno_nontrapping_fptoint : Flag<["-"], "mno-nontrapping-fptoint">, Group<m_wasm_Features_Group>;
 def msign_ext : Flag<["-"], "msign-ext">, Group<m_wasm_Features_Group>;
 def mno_sign_ext : Flag<["-"], "mno-sign-ext">, Group<m_wasm_Features_Group>;
 def mexception_handing : Flag<["-"], "mexception-handling">, Group<m_wasm_Features_Group>;

include/clang/Frontend/CodeGenOptions.h

public:
   };
 
   enum SignReturnAddressScope {
     None,    // No signing for any function
     NonLeaf, // Sign the return address of functions that spill LR
     All      // Sign the return address of all functions
   };
 
+  enum SignReturnAddressKeyValue { AKey, BKey };
+
   /// The code model to use (-mcmodel).
   std::string CodeModel;
 
   /// The filename with path we use for coverage data files. The runtime
   /// allows further manipulation with the GCOV_PREFIX and GCOV_PREFIX_STRIP
   /// environment variables.
   std::string CoverageDataFile;
 

include/clang/Frontend/CodeGenOptions.def

 
 /// Whether to emit all vtables
 CODEGENOPT(ForceEmitVTables, 1, 0)
 
 /// Whether to emit an address-significance table into the object file.
 CODEGENOPT(Addrsig, 1, 0)
 
 ENUM_CODEGENOPT(SignReturnAddress, SignReturnAddressScope, 2, None)
+ENUM_CODEGENOPT(SignReturnAddressKey, SignReturnAddressKeyValue, 1, AKey)
 
 /// Whether to emit unused static constants.
 CODEGENOPT(KeepStaticConsts, 1, 0)
 
 #undef CODEGENOPT
 #undef ENUM_CODEGENOPT
 #undef VALUE_CODEGENOPT
 

lib/CodeGen/CGDeclCXX.cpp

if (getLangOpts().Sanitize.has(SanitizerKind::SafeStack) &&
     Fn->addFnAttr(llvm::Attribute::SafeStack);
 
   if (getLangOpts().Sanitize.has(SanitizerKind::ShadowCallStack) &&
       !isInSanitizerBlacklist(SanitizerKind::ShadowCallStack, Fn, Loc))
     Fn->addFnAttr(llvm::Attribute::ShadowCallStack);
 
   auto RASignKind = getCodeGenOpts().getSignReturnAddress();
   if (RASignKind != CodeGenOptions::SignReturnAddressScope::None)
+  {
     Fn->addFnAttr("sign-return-address",
                   RASignKind == CodeGenOptions::SignReturnAddressScope::All
                       ? "all"
                       : "non-leaf");
+    auto RASignKey = getCodeGenOpts().getSignReturnAddressKey();
+    Fn->addFnAttr("sign-return-address-key",
+                  RASignKey == CodeGenOptions::SignReturnAddressKeyValue::AKey
+                      ? "a_key"
+                      : "b_key");
+  }
+
   return Fn;
 }
 
 /// Create a global pointer to a function that will initialize a global
 /// variable.  The user has requested that this pointer be emitted in a specific
 /// section.
 void CodeGenModule::EmitPointerToInitFunc(const VarDecl *D,
                                           llvm::GlobalVariable *GV,

lib/CodeGen/TargetInfo.cpp

void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
     auto Kind = CGM.getCodeGenOpts().getSignReturnAddress();
     if (Kind == CodeGenOptions::SignReturnAddressScope::None)
       return;
 
     Fn->addFnAttr("sign-return-address",
                   Kind == CodeGenOptions::SignReturnAddressScope::All
                       ? "all"
                       : "non-leaf");
+
+    auto Key = CGM.getCodeGenOpts().getSignReturnAddressKey();
+    Fn->addFnAttr("sign-return-address-key",
+                  Key == CodeGenOptions::SignReturnAddressKeyValue::AKey
+                      ? "a_key"
+                      : "b_key");
   }
 };
 
 class WindowsAArch64TargetCodeGenInfo : public AArch64TargetCodeGenInfo {
 public:
   WindowsAArch64TargetCodeGenInfo(CodeGenTypes &CGT, AArch64ABIInfo::ABIKind K)
       : AArch64TargetCodeGenInfo(CGT, K) {}
 

lib/Driver/ToolChains/Clang.cpp

void Clang::RenderTargetOptions(const llvm::Triple &EffectiveTriple,
 
   case llvm::Triple::wasm32:
   case llvm::Triple::wasm64:
     AddWebAssemblyTargetArgs(Args, CmdArgs);
     break;
   }
 }
 
 void Clang::AddAArch64TargetArgs(const ArgList &Args,
                                  ArgStringList &CmdArgs) const {
   const llvm::Triple &Triple = getToolChain().getEffectiveTriple();
 
   if (!Args.hasFlag(options::OPT_mred_zone, options::OPT_mno_red_zone, true) ||
       Args.hasArg(options::OPT_mkernel) ||
       Args.hasArg(options::OPT_fapple_kext))
     CmdArgs.push_back("-disable-red-zone");
 
   if (!Args.hasFlag(options::OPT_mimplicit_float,
                     options::OPT_mno_implicit_float, true))
     CmdArgs.push_back("-no-implicit-float");
 
   const char *ABIName = nullptr;
   if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ))
     ABIName = A->getValue();
   else if (Triple.isOSDarwin())
     ABIName = "darwinpcs";
   else
     ABIName = "aapcs";
 
   CmdArgs.push_back("-target-abi");
   CmdArgs.push_back(ABIName);
 
   if (Arg *A = Args.getLastArg(options::OPT_mfix_cortex_a53_835769,
                                options::OPT_mno_fix_cortex_a53_835769)) {
     CmdArgs.push_back("-mllvm");
     if (A->getOption().matches(options::OPT_mfix_cortex_a53_835769))
       CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=1");
     else
       CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=0");
   } else if (Triple.isAndroid()) {
     // Enabled A53 errata (835769) workaround by default on android
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=1");
   }
 
   // Forward the -mglobal-merge option for explicit control over the pass.
   if (Arg *A = Args.getLastArg(options::OPT_mglobal_merge,
                                options::OPT_mno_global_merge)) {
     CmdArgs.push_back("-mllvm");
     if (A->getOption().matches(options::OPT_mno_global_merge))
       CmdArgs.push_back("-aarch64-enable-global-merge=false");
     else
       CmdArgs.push_back("-aarch64-enable-global-merge=true");
   }
 
-  if (Arg *A = Args.getLastArg(options::OPT_msign_return_address)) {
+  if (Arg *A = Args.getLastArg(options::OPT_msign_return_address_EQ)) {
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-msign-return-address=") + A->getValue()));
   }
 }
 
 void Clang::AddMIPSTargetArgs(const ArgList &Args,
                               ArgStringList &CmdArgs) const {
   const Driver &D = getToolChain().getDriver();
   StringRef CPUName;
   StringRef ABIName;
   const llvm::Triple &Triple = getToolChain().getTriple();
   mips::getMipsCPUAndABI(Args, Triple, CPUName, ABIName);
 
   CmdArgs.push_back("-target-abi");
   CmdArgs.push_back(ABIName.data());
 
   mips::FloatABI ABI = mips::getMipsFloatABI(D, Args);
   if (ABI == mips::FloatABI::Soft) {

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
 
   Opts.EmitCheckPathComponentsToStrip = getLastArgIntValue(
       Args, OPT_fsanitize_undefined_strip_path_components_EQ, 0, Diags);
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
   Opts.Addrsig = Args.hasArg(OPT_faddrsig);
 
-  if (Arg *A = Args.getLastArg(OPT_msign_return_address)) {
-    StringRef SignScope = A->getValue();
+  if (Arg *A = Args.getLastArg(OPT_msign_return_address_EQ)) {
+    const auto SignScopeKey = StringRef(A->getValue()).split('+');
+    StringRef SignScope = SignScopeKey.first;
+    StringRef SignKey = SignScopeKey.second;
+
     if (SignScope.equals_lower("none"))
       Opts.setSignReturnAddress(CodeGenOptions::SignReturnAddressScope::None);
     else if (SignScope.equals_lower("all"))
       Opts.setSignReturnAddress(CodeGenOptions::SignReturnAddressScope::All);
     else if (SignScope.equals_lower("non-leaf"))
       Opts.setSignReturnAddress(
           CodeGenOptions::SignReturnAddressScope::NonLeaf);
     else
       Diags.Report(diag::err_drv_invalid_value)
-          << A->getAsString(Args) << A->getValue();
+          << A->getAsString(Args) << SignScope;
+
+    if (!SignScope.empty() && !SignKey.empty()) {
+      if (SignKey.equals_lower("a_key"))
+        Opts.setSignReturnAddressKey(CodeGenOptions::SignReturnAddressKeyValue::AKey);
+      else if (SignKey.equals_lower("b_key"))
+        Opts.setSignReturnAddressKey(CodeGenOptions::SignReturnAddressKeyValue::BKey);
+      else
+        Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args)
+                                                  << SignKey;
+    }
   }
 
   Opts.KeepStaticConsts = Args.hasArg(OPT_fkeep_static_consts);
 
   Opts.SpeculativeLoadHardening = Args.hasArg(OPT_mspeculative_load_hardening);
 
   return Success;
 }

test/CodeGen/aarch64-sign-return-address.c

 // RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=none  %s | FileCheck %s --check-prefix=CHECK-NONE
 // RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=non-leaf %s | FileCheck %s --check-prefix=CHECK-PARTIAL
 // RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=all %s | FileCheck %s --check-prefix=CHECK-ALL
+// RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=all+a_key %s | FileCheck %s --check-prefix=CHECK-A-KEY
+// RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=all+b_key %s | FileCheck %s --check-prefix=CHECK-B-KEY
 
 // CHECK-NONE: @foo() #[[ATTR:[0-9]*]]
 // CHECK-NONE-NOT: attributes #[[ATTR]] = { {{.*}} "sign-return-address"={{.*}} }}
 
 // CHECK-PARTIAL: @foo() #[[ATTR:[0-9]*]]
-// CHECK-PARTIAL: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="non-leaf" {{.*}}}
+// CHECK-PARTIAL: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="non-leaf" "sign-return-address-key"="a_key" {{.*}}}
 
 // CHECK-ALL: @foo() #[[ATTR:[0-9]*]]
-// CHECK-ALL: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="all" {{.*}} }
+// CHECK-ALL: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="all" "sign-return-address-key"="a_key" {{.*}} }
+
+// CHECK-A-KEY: @foo() #[[ATTR:[0-9]*]]
+// CHECK-A-KEY: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="all" "sign-return-address-key"="a_key" {{.*}} }
+
+// CHECK-B-KEY: @foo() #[[ATTR:[0-9]*]]
+// CHECK-B-KEY: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="all" "sign-return-address-key"="b_key" {{.*}} }
 
 void foo() {}

test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp

 // RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=none  %s | \
 // RUN: FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-NONE
 // RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=non-leaf %s | \
 // RUN: FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-PARTIAL
 // RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=all %s | \
 // RUN: FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-ALL
+// RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=all+a_key %s | \
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-A-KEY
+// RUN: %clang -target aarch64-arm-none-eabi -S -emit-llvm -o - -msign-return-address=all+b_key %s | \
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-B-KEY
 
 struct Foo {
   Foo() {}
   ~Foo() {}
 };
 
 Foo f;
 
 // CHECK: @llvm.global_ctors {{.*}}i32 65535, void ()* @[[CTOR_FN:.*]], i8* null
 
 // CHECK: @[[CTOR_FN]]() #[[ATTR:[0-9]*]]
 
 // CHECK-NONE-NOT: attributes #[[ATTR]] = { {{.*}} "sign-return-address"={{.*}} }}
-// CHECK-PARTIAL: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="non-leaf" {{.*}}}
-// CHECK-ALL: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="all" {{.*}} }
+// CHECK-PARTIAL: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="non-leaf" "sign-return-address-key"="a_key" {{.*}}}
+// CHECK-ALL: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="all" "sign-return-address-key"="a_key" {{.*}} }
+// CHECK-A-KEY: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="all" "sign-return-address-key"="a_key" {{.*}} }
+// CHECK-B-KEY: attributes #[[ATTR]] = { {{.*}} "sign-return-address"="all" "sign-return-address-key"="b_key" {{.*}} }