This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] - Generate pointer authentication instructions
ClosedPublic

Authored by LukeCheeseman on Jul 25 2018, 5:30 AM.

Details

Summary

Generate pointer authentication instructions

  • The functions instrumented depend on function attribtues: all (all functions instrumentent), non-leaf (only those that spill LR), none
  • Function epilogues sign the LR before spilling to the stack and authenticate the LR once restored
  • If the target is v8.3a or greater than can use the combined authenticate and return instruction

Diff Detail

Event Timeline

LukeCheeseman created this revision.Jul 25 2018, 5:30 AM
javed.absar added inline comments.Jul 25 2018, 6:53 AM
lib/Target/AArch64/AArch64FrameLowering.cpp
299

Please add reason string to the assert e.g. asset(... && "expected one of none|all|partial");

LukeCheeseman marked an inline comment as done.
olista01 added inline comments.
lib/Target/AArch64/AArch64FrameLowering.cpp
286

This comment and the gcc/clang command line options use "non-leaf", but you check for "partial" in the function attribute. I think it would be better to use "non-leaf" everywhere, unless there's a good reason to switch to "partial".

293

I can't find any other places where we do case-insensitive checks of function attributes, so I think we should stick to doing an exact string-compare here.

880

I think this could do with a comment explaining the different pre-v8.3 and post-v8.3 code.

test/CodeGen/AArch64/sign-return-address.ll
7

Most of the code in these tests can be removed (e.g. this one could be "ret i32 %x", or just "ret void").

69

It would be clearer to add an external function to call, rather than having the tests call each other.

113

Putting these attributes directly on the function definitions (in place of the #<n> reference) would make the tests a bit easier to read.

What happens if a function with signed LR tail calls a function with unsigned LR, or vice versa?

  • Address reviewer points
  • Add a test for when a tail call occurs in a function that signs the LR
LukeCheeseman marked 6 inline comments as done and an inline comment as not done.Jul 26 2018, 1:38 AM

What happens if a function with signed LR tail calls a function with unsigned LR, or vice versa?

I've added a test for what I think is meant here, see spill_lr_and_tail call.

The LR is authenticated before the tail call happens.

LukeCheeseman marked an inline comment as done.Jul 26 2018, 2:30 AM
ab added a subscriber: ab.Jul 26 2018, 10:41 AM

A few minor comments inline, and one question: it sounds like this is based on a GCC feature; what's the expected behavior for __builtin_returnaddress ? Shouldn't it xpaci the result?

Also, that's one of the reasons why I'm not sure it's wise to emit the instructions always, even if we're not explicitly targeting v8.3a. You run the risk of having some parts of the code (__builtin_returnaddress, backtrace, unwind, ...) be built for v8, not knowing that they need to strip, even though some other part might have enabled this LR signing. I'm not sure there's much to be done about that, though ;)

lib/Target/AArch64/AArch64FrameLowering.cpp
297

Is this actually necessary? "all" is still only functions that actually save LR, no? Why sign it if it's not saved?

1007

One trick to make sure this covers all 'return' paths (even when the code changes) is to use 'make_scope_exit' to guarantee the code is run.

test/CodeGen/AArch64/sign-return-address.ll
6

To expand on Oliver's comments, you can also remove the basic block names, and 'dso_local' (unless its semantics matter, which doesn't seem to be the case here)

Also, another nit: having the check lines away from the check-label is kinda confusing, maybe put it all before the IR?

43

The ordering of the instructions is critical here, no? Whether the PAC/AUT occur before (resp. after) SP updates (or, for that matter, the LR save) is crucial to the feature. Maybe have explicit CHECK-NEXT for the rest of the prologue/epilogue?

Update patch based on comments

LukeCheeseman marked 3 inline comments as done.Jul 27 2018, 7:45 AM
LukeCheeseman added inline comments.
lib/Target/AArch64/AArch64FrameLowering.cpp
297

Yes, it is necessary. "all" is all functions, even those that don't save the LR. Assume you are in a context where you have managed to gain control over the flow of execution, if you don't sign the LR in any function that doesn't save the LR then these functions becomes the ideal place to find gadgets.

lib/Target/AArch64/AArch64FrameLowering.cpp
289

nit: clang-format this change

Ran through clang-format

When the basic block has no terminator (as it will fall through to the next block), don't bail out of inserting an authentication instruction.

olista01 accepted this revision.Aug 17 2018, 5:33 AM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 17 2018, 5:33 AM
emaste added a subscriber: emaste.Sep 18 2018, 2:13 PM
LukeCheeseman closed this revision.Sep 26 2018, 3:53 AM

This was closed by r340018 (I attached the wrong differential revision to the commit and so this was not automatically closed)