Page MenuHomePhabricator

[AArch64][v8.3A] Avoid inserting implicit landing pads (PACI*SP)
ClosedPublic

Authored by pbarrio on May 5 2021, 9:26 AM.

Details

Summary

PACI*SP have the advantage that they are in HINT space, meaning
they can be run successfully in hardware without PAuth support -
they will just behave as a NOP. However, PACI*SP are also implicit
landing pads (think of an extra BTI jc). Therefore, they allow
indirect jumps of all kinds into them, potentially inserting new
gadgets. This patch replaces PACI*SP by PACI* LR, SP when
compiling explicitly for hardware with full PAuth support. PACI*
is not in the HINT space, therefore it will fault when run in
hardware without PAuth support, but it is also not a landing pad,
making programs safer in newer HW.

Diff Detail

Event Timeline

pbarrio created this revision.May 5 2021, 9:26 AM
pbarrio requested review of this revision.May 5 2021, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2021, 9:26 AM

Wondering if AArch64InstrInfo::getOutliningType and the outline should do the same because it might emit PACIASP.
see the old change:
https://reviews.llvm.org/D77565?id=256025#change-SC49e8S7FnMX

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
1144

maybe.

pbarrio updated this revision to Diff 344374.May 11 2021, 5:35 AM

Added handling of outlining for PACI* similarly to PACI*SP etc, and RegState info to SP.

pbarrio marked an inline comment as done.May 11 2021, 5:38 AM
pbarrio added a reviewer: ab.May 13 2021, 5:41 AM
pbarrio updated this revision to Diff 345102.May 13 2021, 5:43 AM

Updated PAC handling in the outliner. Now all outlined functions should use v8.3a instructions where possible.

All comments addressed (thank you @danielkiss). Does it look reasonable now?

efriedma added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
6945

This isn't necessary, I think. The PACIA has an explicit "LR" operand, which should block outlining anyway.

pbarrio edited reviewers, added: efriedma; removed: eli.friedman.Jun 15 2021, 6:45 AM
pbarrio updated this revision to Diff 352463.Jun 16 2021, 9:31 AM

@efriedma thank you for the review. Commit updated without the special outlining handling.

pbarrio marked an inline comment as done.Jun 16 2021, 9:34 AM
pbarrio added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
6945

You are right. I remember I broke the outlining tests at some point and I thought this special-casing fixed them. The tests below are now passing without the special cases, though, so I may be imagining things.

This revision is now accepted and ready to land.Jun 16 2021, 11:44 AM
pbarrio updated this revision to Diff 353392.Jun 21 2021, 9:16 AM
pbarrio marked an inline comment as done.

The following commit added a register in the definition of the PAC instructions:

https://reviews.llvm.org/rGea0eec69f16e0f1b00fec413986e4e44f6f627fa

Patch updated to add the register when building the MI.

I'll leave the review open for a couple of days in case anyone sees a problem with the additional .addReg(AArch64::LR), otherwise I'll commit.

danielkiss accepted this revision.Jun 22 2021, 9:35 AM

LGTM, thanks!

This revision was landed with ongoing or failed builds.Jun 24 2021, 10:25 AM
This revision was automatically updated to reflect the committed changes.