This is an archive of the discontinued LLVM Phabricator instance.

[llvm][AArch64] Fix an interaction of SLS and BTI after a returns twice call
ClosedPublic

Authored by DavidSpickett on Feb 13 2023, 7:31 AM.

Details

Summary

This fixes the combination of two things:

  • Placing a BTI after calls to a returns twice function like setjmp. This allows the setjmp to return with a br instead of a ret.
  • Straight line speculation mitigations that replace BLR with a BL to a thunk that does the mitigation, and then goes to the original target.

Originally I marked AArch64call_bti as requiring that SLS mitigation
be disabled. This caused a crash when you tried to codegen with both.
Since CALL_BTI tried to match with AArch64call_bti but could not.

This change does 2 things:

  • Follow the pattern set by AArch64call and add 2 patterns for AArch64call_bti. One with no IP (interprocedural) registers, and one with. For SLS mitigation on and off respectively.
  • Modify the sls hardening pass to iterate through bundled instructions, as the AArch64 KCFI pass does.

Since there is a 1:1 replacement of the BLR with a BL,
the bundle remains intact. This is checked with an MIR test.

The ir -> asm testing is updated to add runs with the sls
mitigation enabled.

Diff Detail

Event Timeline

DavidSpickett created this revision.Feb 13 2023, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 7:31 AM
DavidSpickett requested review of this revision.Feb 13 2023, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 7:31 AM
DavidSpickett added a subscriber: pzheng.

I should note that the IR that describes an indirect call to a returns twice function will not be generated by current clang. It seems like the SROA pass drops that attribute when it replaces the value. Perhaps it did at one point do that, as I don't think I hand wrote the IR. I will see if that is a bug or not.

That doesn't prevent this patch going in though.

I asked my colleagues about the merits of pseudos that are expanded late, vs. having passes look inside bundles. There were a lot of opinions mostly toward bundles being a bad way to do this. However, it's what we have and the KCFI pass already handles something like this so I went with it. Let's teach 1 pass to handle a bundle rather than 2 how to handle a pseudo.

@pzheng Please check whatever build configuration you were hitting the crash in. I think I'm testing all of SelectionDag/FastIsel/GlobalIsel but not 100% sure.

llvm/test/CodeGen/AArch64/setjmp-bti.ll
4

And now I see I forgot a SelectionDAG test for this one.

DavidSpickett added inline comments.Feb 13 2023, 7:39 AM
llvm/test/CodeGen/AArch64/speculation-hardening-sls-blr-bti.mir
74

Note that the sls thunk is transparent such that when the setjmp returns it will hit this bti, not to somewhere in the thunk.

Add SelectionDAG test for bti+sls.

DavidSpickett edited the summary of this revision. (Show Details)Feb 13 2023, 7:59 AM
DavidSpickett added inline comments.Feb 13 2023, 8:04 AM
llvm/test/CodeGen/AArch64/setjmp-bti.ll
4

Added.

pzheng accepted this revision.Feb 13 2023, 4:59 PM

Thanks for the fix, @DavidSpickett! I confirm that this patch fixes the crash we encountered.

This revision is now accepted and ready to land.Feb 13 2023, 4:59 PM

I should note that the IR that describes an indirect call to a returns twice function will not be generated by current clang. It seems like the SROA pass drops that attribute when it replaces the value. Perhaps it did at one point do that, as I don't think I hand wrote the IR. I will see if that is a bug or not.

Filed https://github.com/llvm/llvm-project/issues/60732. It is a generic issue with SROA I think.

kristof.beyls accepted this revision.Feb 14 2023, 2:46 AM

LGTM, thank you!