This is an archive of the discontinued LLVM Phabricator instance.

[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
1126

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
6841 ↗(On Diff #345102)

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
6841 ↗(On Diff #345102)

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.

This change claims that PACIASP has an implicit BTI JC. Is that correct? From my reading of the spec (and I have about 50% certainty that I'm right), the behavior is actually the same as of BTI C. At least this is true on Linux, where SCTRL_EL1.BT0 is set to 1.

I'm asking because -mbranch-protection=standard emits shorter code sequence with -march=armv8.2-a compared to -march=armv8.3-a. See

0:   d503233f        paciasp

vs

0:   d503245f        bti     c
4:   dac103fe        pacia   x30, sp
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2023, 2:37 PM

You are right, yes, the implicit landing pad in PACIxSP is configurable through SCTRL_EL1.BT0. It can behave either as BTI c or BTI jc. Since the value of a sysreg is not known at compile time, I took the safe approach here and assumed that PACIxSP could allow all kinds of jumps and therefore they would be unsafe: better have an explicit BTI c than an implicit BTI jc.

We could take the efficient approach and go back to emitting PACIxSP. It may not be a problem in practice if we are reasonably sure that SCTRL_EL1.BT0 will be 1 in most cases. Worst case, we could allow changing the default behaviour with a CLI flag (e.g. adding another value for -mbranch-protection), or with the triple's ABI field if the decision will be taken by the platform ABI (i.e. --target=*-linux)?

I don't have a strong opinion.

I think using the environment part of the triple is the right choice. We could also just go ahead and flip the current behavior until there is evidence of a platform that configures SCTRL_EL1.BT0 differently, at which point they can implement the triple logic.

This has non-trivial code size impact (0.5 to 1%).

Linux kernel sets the the BT0 (and BT1) to 1 unconditionally.
This results the When the PE is executing at EL0, PACIASP and PACIBSP are not compatible with PSTATE.BTYPE == 0b11. (ArmARM)
That means PACI*SP is just a BTI C from BTI point of view.

I think right now changing the default is reasonable. (reverting this patch)
Maybe just add new subtarget feature that represent this BT flag and change the above Subtarget.hasPAuth to Subtarget.hasBTGoodName.. which is false until needed by a target.

I've uploaded D141978 with a revert. I've opted against introducing a new subtarget feature because that would mean keeping dead code in the repo, without a way of testing it.