Page MenuHomePhabricator

[AArch64] Return Address Signing B Key Support
ClosedPublic

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

Details

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

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.
test/CodeGen/aarch64-sign-return-address.c
1–9

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
include/clang/Frontend/CodeGenOptions.h
117

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
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 added inline comments.Oct 9 2018, 5:09 AM
lib/Frontend/CompilerInvocation.cpp
1143

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
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.

373

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

lib/CodeGen/TargetInfo.cpp
4995

Style nit: brace on same line as if.

lib/Driver/ToolChains/Clang.cpp
1433

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

1440

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

1448

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

1460

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.

1547

Nit: brace on same line as if.

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

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)