Page MenuHomePhabricator

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

Authored by zbrid on Tue, Mar 10, 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.Tue, Mar 10, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Mar 10, 10:05 AM
zbrid updated this revision to Diff 249446.Tue, Mar 10, 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.Tue, Mar 10, 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.Wed, Mar 11, 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
184

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.Wed, Mar 11, 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.Thu, Mar 12, 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.Thu, Mar 12, 4:39 PM

Rm forward declaration

zbrid added a comment.EditedThu, Mar 12, 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
2

Nit: one line is sufficient here

111

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