Page MenuHomePhabricator

[AArch64] Return Address Signing B Key Support

Authored by LukeCheeseman on Aug 29 2018, 8:16 AM.


  • 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

Diff Detail

Event Timeline

LukeCheeseman created this revision.Aug 29 2018, 8:16 AM
LukeCheeseman edited the summary of this revision. (Show Details)Aug 29 2018, 8:17 AM
SjoerdMeijer added inline comments.

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

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

  • 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 marked an inline comment as done.Sep 14 2018, 3:54 AM
emaste added a subscriber: emaste.Sep 24 2018, 8:22 AM
  • 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 added inline comments.Oct 1 2018, 8:28 AM

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

LukeCheeseman added inline comments.Oct 1 2018, 8:41 AM

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

olista01 added inline comments.Oct 9 2018, 5:09 AM

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.

  • 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 added inline comments.Oct 17 2018, 1:21 PM

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.


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


Style nit: brace on same line as if.


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


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


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


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.


Nit: brace on same line as if.


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

LukeCheeseman marked 9 inline comments as done.
olista01 accepted this revision.Oct 25 2018, 1:39 AM

LGTM, thanks!

This revision is now accepted and ready to land.Oct 25 2018, 1:39 AM
LukeCheeseman closed this revision.Oct 25 2018, 8:27 AM

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