Page MenuHomePhabricator

[AArch64] Introduce AArch64SLSHardeningPass, implementing hardening of RET and BR instructions.
ClosedPublic

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

Details

Summary

Some processors may speculatively execute the instructions immediately
following RET (returns) and BR (indirect jumps), even though
control flow should change unconditionally at these instructions.
To avoid a potential miss-speculatively executed gadget after these
instructions leaking secrets through side channels, this pass places a
speculation barrier immediately after every RET and BR instruction.

Since these barriers are never on the correct, architectural execution
path, performance overhead of this is expected to be low.

On targets that implement that Armv8.0-SB Speculation Barrier extension,
a single SB instruction is emitted that acts as a speculation barrier.
On other targets, a DSB SYS followed by a ISB is emitted to act as a
speculation barrier.

In the implementation, these speculation barriers are implemented as
pseudo instructions to avoid later passes to analyze them and
potentially remove them.

Even though currently LLVM does not produce BRAA/BRAB/BRAAZ/BRABZ
instructions, these are also mitigated by the pass and tested through a
MIR test.

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

Diff Detail

Event Timeline

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

The pre-commit test failures are in the newly added test, so need to be fixed before this can be committed. My inline comments could be addressed as follow up patches given that this is related to a recently-published security vulnerability.

llvm/lib/Target/AArch64/AArch64InstrInfo.h
388

There's a isIndirectBranch flag on instructions in the tablegen, so is this needed?

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

Since we only care about instructions which are terminators, could this me made more efficient by just scanning the terminators?

llvm/test/CodeGen/AArch64/speculation-hardening-sls.ll
23

I assume that the return instruction is being split into two in the backend somewhere, but it would be clearer to just have two returns in the IR. If this is testing for a bug triggered when that optimisation happening, then this could do with a comment explaining that.

kristof.beyls marked 3 inline comments as done.
kristof.beyls marked 3 inline comments as done.Jun 9 2020, 8:55 AM

Thanks for the feedback!

The pre-commit test failures in the newly added tests seem to have been caused by the parent patch D81399 not having been committed before.
That has been committed since.
So if the pre-commit test infrastructure also runs tests on patch updates, it will hopefully show the tests passing now.

llvm/lib/Target/AArch64/AArch64InstrInfo.h
388

When I change the one previous use of isIndirectBranchOpcode in AArch64InstrInfo::analyzeBranch to use isIndirectBranch, I see 2 tests starting to fail:

LLVM :: CodeGen/AArch64/callbr-asm-obj-file.ll
LLVM :: CodeGen/AArch64/callbr-asm-label.ll

The change in behaviour seems to be caused by the INLINEASM_BR being an IndirectBranch, but not recognized by isIndirectBranchOpcode as such.
Maybe this indicates a missed optimization (and makes the case for not having isIndirectBranchOpcode and its duplication).
But given it's a change in behaviour, I think it's best to do a change for this in a separate patch.

Also, analyzeBranch uses IsIndirectBranchOpcode, isCondBranchOpcode, isUncondBranchOpcode together, so maybe it's more consistent for it to keep on using a set of interfaces all based on opcodes; not sure.

Anyway, while I agree it would be better not to repeat the same information as the isIndirectBranch on instructions in tablegen, with the above, it seems best to tackle this seeming duplication as a separate patch.

I've added a test that does contain/produce INLINEASM_BR.

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

Good point, and thanks for pointing me to getFirstTerminator().

llvm/test/CodeGen/AArch64/speculation-hardening-sls.ll
23

Fair point, I'll make the change.

This revision is now accepted and ready to land.Jun 10 2020, 1:39 AM
This revision was automatically updated to reflect the committed changes.
kristof.beyls marked 2 inline comments as done.