This is an archive of the discontinued LLVM Phabricator instance.

[x86][seses] Introduce SESES pass for LVI
ClosedPublic

Authored by zbrid on Mar 10 2020, 10:05 AM.

Details

Summary

This is an implementation of Speculative Execution Side Effect
Suppression which is intended as a last resort mitigation against Load
Value Injection, LVI, a newly disclosed speculative execution side
channel vulnerability.

One pager:
https://software.intel.com/security-software-guidance/software-guidance/load-value-injection

Deep dive:
https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection

The mitigation consists of a compiler pass that inserts an LFENCE before
each memory read instruction, memory write instruction, and the first
branch instruction in a group of terminators at the end of a basic
block. The goal is to prevent speculative execution, potentially based
on misspeculated conditions and/or containing secret data, from leaking
that data via side channels embedded in such instructions.

This is something of a last-resort mitigation: it is expected to have
extreme performance implications and it may not be a complete mitigation
due to trying to enumerate side channels.

In addition to the full version of the mitigation, this patch
implements three flags to turn off part of the mitigation. These flags
are disabled by default. The flags are not intended to result in a
secure variant of the mitigation. The flags are intended to be used by
users who would like to experiment with improving the performance of
the mitigation. I ran benchmarks with each of these flags enabled in
order to find if there was any room for further optimization of LFENCE
placement with respect to LVI.

Performance Testing Results

When applying this mitigation to BoringSSL, we see the following
results. These are a summary/aggregation of the performance changes when
this mitigation is applied versus when no mitigation is applied.

Fully Mitigated vs Baseline
Geometric mean
0.071 (Note: This can be read as the ops/s of the mitigated
program was 7.1% of the ops/s of the unmitigated program.)
Minimum
0.041
Quartile 1
0.060
Median
0.063
Quartile 3
0.077
Maximum
0.230

Diff Detail

Event Timeline

zbrid created this revision.Mar 10 2020, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2020, 10:05 AM
zbrid updated this revision to Diff 249446.Mar 10 2020, 10:34 AM

Add link to bug to FIXME

Thanks so much for working on this!

Keeping in mind that I'm not an x86 backend expert, this approach and implementation (especially allowing people to experiment with different flags) look reasonable to me. Please wait for approval from one of the other reviewers before landing.

This revision is now accepted and ready to land.Mar 10 2020, 5:04 PM

Haven't looked into the code, but some suggestions for the test. Insignificant parts should be deleted. For example, .note.GNU-stack is not needed. If the codegen has nothing to do with CFI, .cfi_startproc and .cfi_endproc should be omitted. I added .Lfoo$local as a dso_local related assembler/linker optimization. It should be omitted. Tricky code sequence in the assembly should be documented. A file level comments helps readers understand what the test is about.

llvm/test/CodeGen/X86/speculative-execution-side-effect-suppression.ll
162

.note.GNU-stack is not needed.

craig.topper added inline comments.Mar 11 2020, 9:36 AM
llvm/lib/Target/X86/X86SpeculativeExecutionSideEffectSuppression.cpp
37

Something like x86-seses-one-lfence-per-bb might make a clearer name?

147

Can this be ordered earlier to avoid the forward declaration?

llvm/test/CodeGen/X86/O3-pipeline.ll
189

Drop " Pass". Doesn't look like most other passes refer to themselves as Pass

What is the intention of this set of patches in relation to D75938? It was unclear to me whether you intended to commit this implementation or were just offering it as an alternative for discussion.

zbrid added a comment.Mar 11 2020, 5:13 PM

What is the intention of this set of patches in relation to D75938? It was unclear to me whether you intended to commit this implementation or were just offering it as an alternative for discussion.

I offer these patches for discussion and will upstream in folks in the LLVM community would like them to be upstreamed. If this approach and the other approach are both desired by the LLVM community, then I will work with Intel to decide whether to merge the approaches into a single framework/pass or not.

If you have thoughts on whether or not this should be upstreamed, please let me know and feel free to add to the discussion.

For code review comments:

  • I will address these shortly.

What is the intention of this set of patches in relation to D75938? It was unclear to me whether you intended to commit this implementation or were just offering it as an alternative for discussion.

I offer these patches for discussion and will upstream in folks in the LLVM community would like them to be upstreamed. If this approach and the other approach are both desired by the LLVM community, then I will work with Intel to decide whether to merge the approaches into a single framework/pass or not.

If you have thoughts on whether or not this should be upstreamed, please let me know and feel free to add to the discussion.

For code review comments:

  • I will address these shortly.

I see it this way. We definitely need D75935 to mitigate RET instructions, and D75934 to mitigate indirect calls/jumps. The SESES patch (with D75944) and D75936+D75937 each use very different approaches to mitigate loads. So the choice is D75939+D75944 against D75936+D75937.

zbrid updated this revision to Diff 250088.Mar 12 2020, 4:30 PM
zbrid marked 3 inline comments as done.
  • Update tests (CHECK-NEXT, rm some extra stuff)
  • ClangFormat
  • Update pass name
  • Change flag to better name
zbrid updated this revision to Diff 250089.Mar 12 2020, 4:39 PM

Rm forward declaration

zbrid added a comment.EditedMar 12 2020, 4:39 PM

Updated based on all the comments.

EDIT: I could do some more work to remove extra stuff from the tests, but I did a bit for now. I'll update those a bit more later. I need to understand FileCheck and LLVM IR better for further changes.

Harbormaster completed remote builds in B49074: Diff 250089.
jdoerfert added inline comments.
llvm/lib/Target/X86/X86SpeculativeExecutionSideEffectSuppression.cpp
3

Nit: one line is sufficient here

112

Style: No braces around single statements. (also elsewhere)

zbrid updated this revision to Diff 260370.Apr 27 2020, 10:27 AM
zbrid marked an inline comment as done.

Update based on jdoerfert's style comments

zbrid marked an inline comment as done.Apr 27 2020, 10:28 AM
zbrid updated this revision to Diff 260383.Apr 27 2020, 11:14 AM

Update all tests to be autogenerated by update_llc_test_checks.py

I don't think that this feature will be secure unless it is also used with -mlvi-cfi. Specifically, it is not sufficient to mitigate a RET simply by placing an LFENCE before it. There must also be a read from RSP's pointee just prior to that LFENCE. Also, indirect calls/jumps from memory must be decomposed into discrete load and call/jump from register operations with an interposed LFENCE. The -mlvi-cfi enables an X86 target feature that performs both of these mitigations correctly.

Also, I think that all of your lit tests for various option combinations can be combined into a single file, with different FileCheck prefixes corresponding to different mitigation configurations.

sconstab added inline comments.Apr 27 2020, 11:55 AM
llvm/lib/Target/X86/X86SpeculativeExecutionSideEffectSuppression.cpp
88

There should probably be a CFI check here, e.g.:

if (!Subtarget.useLVIControlFlowIntegrity()) {
  report_fatal_error("SESES must be used with -mlvi-cfi", false);
}

I don't think that this feature will be secure unless it is also used with -mlvi-cfi. Specifically, it is not sufficient to mitigate a RET simply by placing an LFENCE before it. There must also be a read from RSP's pointee just prior to that LFENCE. Also, indirect calls/jumps from memory must be decomposed into discrete load and call/jump from register operations with an interposed LFENCE. The -mlvi-cfi enables an X86 target feature that performs both of these mitigations correctly.

Also, I think that all of your lit tests for various option combinations can be combined into a single file, with different FileCheck prefixes corresponding to different mitigation configurations.

  • Good point on the tests, I'll update them accordingly.
  • Also thanks for reminding me about the -mlvi-cfi flag. I'll add a change to enable that along with this pass.
zbrid updated this revision to Diff 260423.Apr 27 2020, 12:57 PM

[seses] Consolidate SESES tests to a single file

zbrid added a comment.Apr 27 2020, 2:47 PM

@sconstab - For this patch, I'm going to update it to mention it doesn't mitigate indirect branches and returns and to that users should add -mlvi-cfi if they'd like that functionality. I'll follow up with a patch that adds the lvi-cfi x86 subtarget feature when seses is enable. That patch touches a lot of new places, so it'll be easier to review in a separate place. I'll add the report fatal error in the follow up patch too.

zbrid updated this revision to Diff 260460.Apr 27 2020, 2:53 PM

[seses] Tell users about not mitigating against indirect branches

  • Also mention how to mitigate against indirect branches
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.May 14 2020, 4:41 PM

(I don't have an opinion on this patch, but gbiv said above "Please wait for approval from one of the other reviewers before landing." and I don't see approval from anyone else on phab. Maybe it was in email on the list and didn't make it here?)