Page MenuHomePhabricator

[AArch64] Extend AArch64SLSHardeningPass to harden BLR instructions.
ClosedPublic

Authored by kristof.beyls on Jun 8 2020, 8:13 AM.

Details

Summary

To make sure that no barrier gets placed on the architectural execution
path, each

BLR x<N>

instruction gets transformed to a

BL __llvm_slsblr_thunk_x<N>

instruction, with llvm_slsblr_thunk_x<N> a thunk that contains
llvm_slsblr_thunk_x<N>:

BR x<N>
<speculation barrier>

Therefore, the BLR instruction gets split into 2; one BL and one BR.
This transformation results in not inserting a speculation barrier on
the architectural execution path.

The mitigation is off by default and can be enabled by the
harden-sls-blr subtarget feature.

As a linker is allowed to clobber X16 and X17 on function calls, the
above code transformation would not be correct in case a linker does so
when N=16 or N=17. Therefore, when the mitigation is enabled, generation
of BLR x16 or BLR x17 is avoided.

As BLRA* indirect calls are not produced by LLVM currently, this does
not aim to implement support for those.

Diff Detail

Event Timeline

kristof.beyls created this revision.Jun 8 2020, 8:13 AM

A few nits, but given the time sensitivity of this the only blocking one is how this will work with PLTs or long-branch veneers,

llvm/lib/Target/AArch64/AArch64SLSHardening.cpp
154

Is it possible for the call to these thunks to go through a long-branch veneer, PLT, etc, which could clobber x16 or x17 before it gets used in the thunk? Maybe we could avoid generating indirect calls using those registers, but we have the exact opposite restriction for tail calls when using BTI (grep for TCRETURNriBTI).

194

Style nit: Variable should be declared further down, when first set.

209

Is this something which could be fixed in ThunkInserter?

233

This comment is a bit misleading, we're not actually splitting the basic block (that's terminology used elsewhere in LLVM for replacing one basic block with two).

kristof.beyls edited the summary of this revision. (Show Details)

The updated patch addresses Oliver's review comments.
Especially, it addresses the review feedback regarding potential clobbering of X16 or X17 when the call to the thunks go through a long veneer.
It addresses this by avoiding generating indirect calls that use X16 or X17.

kristof.beyls marked 6 inline comments as done.Jun 10 2020, 7:18 AM
kristof.beyls added inline comments.
llvm/lib/Target/AArch64/AArch64SLSHardening.cpp
209

Maybe, but I think that should be done as a separate patch as it's probably target-independent.

233

Hopefully the new comment is better.

ostannard accepted this revision.Jun 10 2020, 8:17 AM

LGTM, with some comments which can be addressed later given the time-sensitivity of this.

llvm/lib/Target/AArch64/AArch64InstrInfo.td
2026

Is the BLRCall pseudo actually needed, or could we just use BLR when not doing the mitigation?

llvm/lib/Target/AArch64/AArch64SLSHardening.cpp
111

BLRCall shouldn't be selected when this pass is enabled, so maybe we should assert here?

154

Please add a comment noting that x16 and x17 are deliberately omitted.

160

X30 doesn't need to be in these lists either, it (correctly) isn't allowed with BLRCallNoIP.

This revision is now accepted and ready to land.Jun 10 2020, 8:17 AM
kristof.beyls marked 2 inline comments as done.
kristof.beyls marked 6 inline comments as done.Jun 11 2020, 6:49 AM
kristof.beyls added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.td
2026

Indeed BLRCall is not needed. Now removed in the latest version of the patch.

llvm/lib/Target/AArch64/AArch64SLSHardening.cpp
111

I've changed the pseudo expansion to use PseudoInstExpansion.
Depending on when that expansion happens in the pipeline (before or after the mitigation pass), one could see BLR or BLRNoIP here.
I thought it's safest to just handle both here. There's an assert in ConvertBLRToBL that should catch what we actually care about, i.e. not using X16/X17 (nor LR).

kristof.beyls marked 3 inline comments as done.Jun 11 2020, 11:41 PM
kristof.beyls added inline comments.
llvm/lib/Target/AArch64/AArch64SLSHardening.cpp
209

I've now posted a fix for this in the ThunkInserter in D81403

This revision was automatically updated to reflect the committed changes.