Page MenuHomePhabricator

Add RET-hardening Support to X86 to mitigate Load Value Injection (LVI) [3/6]
ClosedPublic

Authored by sconstab on Mar 10 2020, 10:00 AM.

Details

Summary

Adding a pass that replaces every ret instruction with the sequence:

pop <scratch-reg>
lfence
jmp *<scratch-reg>

where <scratch-reg> is some available scratch register, according to the
calling convention of the function being mitigated.

Diff Detail

Event Timeline

sconstab created this revision.Mar 10 2020, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2020, 10:00 AM

Can you use the "Edit Related Revisions" link to set the parent/child relationships of these patches and put "[1/5]" in the titles?

sconstab retitled this revision from Add RET-hardening Support to X86 to mitigate Load Value Injection (LVI) to Add RET-hardening Support to X86 to mitigate Load Value Injection (LVI) [3/5].Mar 11 2020, 12:35 PM
sconstab updated this revision to Diff 249770.Mar 11 2020, 3:25 PM

In the case where there is no scratch register available, changed from using OR 0 to SHL 0 to load/store from/to RSP. The benefit of SHL 0 is that it does not clobber EFLAGS.

craig.topper added inline comments.Mar 11 2020, 3:38 PM
llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
122

LLVM doesn't know that shifting by zero doesn't update flags. so I don't know that this really changed anything. Are the flags causing an issue?

sconstab added inline comments.Mar 12 2020, 1:13 PM
llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
122

The code that wase here previously was:

addRegOffset(BuildMI(MBB, Fence, DebugLoc(), TII->get(X86::OR64mi8)),
             X86::RSP, false, 0)
    .addImm(0)
    ->addRegisterDead(X86::EFLAGS, TRI);

I thought that if you don't specify addRegisterDead(EFLAGS) then LLVM will assume that EFLAGS is still live.

The second reason to make this update is to keep consistent with the mitigations performed by GNU AS, which will be updated to use SHL 0.

sconstab updated this revision to Diff 250063.Mar 12 2020, 2:45 PM

Re-added

->addRegisterDead(X86::EFLAGS, TRI);

To address Craig's comment.

sconstab retitled this revision from Add RET-hardening Support to X86 to mitigate Load Value Injection (LVI) [3/5] to Add RET-hardening Support to X86 to mitigate Load Value Injection (LVI) [3/6].Mar 16 2020, 9:30 AM
zbrid added a comment.Mar 17 2020, 5:16 PM

Thanks! Looks great. Please wait for another LGTM from someone more versed in LLVM conventions, but LGTM.

llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
72

I think this should be at the top of the function.

133

nit: Would it make sense to have this incremented when you set Modified to true instead of checking Modified here?

llvm/test/CodeGen/X86/O0-pipeline.ll
77

nit: Same as prior pass: remove "pass" from the name and update here and in the other test for the O3 pipeline

sconstab updated this revision to Diff 251232.Mar 18 2020, 5:32 PM

Addressed Zola's comments.

sconstab marked 6 inline comments as done.Mar 18 2020, 5:34 PM
sconstab added inline comments.
llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
122

Decided to keep addRegisterDead(X86::EFLAGS, TRI)

133

I don't think so. Then the value might be incremented more than once per function, e.g., if more than one BB has a RET.

This revision is now accepted and ready to land.Apr 2 2020, 9:38 AM
This revision was automatically updated to reflect the committed changes.
mattdr added a subscriber: mattdr.Apr 9 2020, 5:23 PM
mattdr added inline comments.
llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
82–83

Does the LLVM optimizer ever invent "custom" calling conventions for nonpublished symbols? Sort of like what's described here: https://devblogs.microsoft.com/oldnewthing/20150128-00/?p=44813

If it does, the assumption here that we can rely on the architecture's calling convention might not hold.

mattdr added inline comments.Apr 9 2020, 5:33 PM
llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
122

If GNU AS and LLVM are using SHL 0, please make sure that https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection is updated as well.

mattdr removed a subscriber: mattdr.

LGTM

sconstab marked 3 inline comments as done.Apr 9 2020, 6:02 PM
sconstab added inline comments.
llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
82–83

This also occurred to me while I was writing the pass, because I know that gcc also does something similar. I looked through the backend to try to find any evidence that LLVM does this optimization, and I came up empty. @craig.topper opinion?

122

Thanks! Will fix.

craig.topper added inline comments.
llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
82–83

We do some optimization in IR in the ArgumentPromotion pass. Its hard to do anything like that example in the backend since you need to rewrite the caller and callee together and we only have the Machine IR for one of those in memory at a time.

There is an Interprocedural Register Allocator infrastructure that attempts to avoid saving registers in the caller than aren't clobbered in the callee. Its not on by default for X86. Not sure how it determines which registers are clobbered. But I think is uses the RegUsageInfoCollector pass. Which probably means the pass in this patch runs too late.

I think @rnk knows more about all the calling convention stuff then me so maybe he can help.

rnk added inline comments.Apr 16 2020, 1:08 PM
llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
82–83

So far as I am aware, no, LLVM does not attempt to create new register parameters that are not part of some known calling convention.

sconstab marked an inline comment as done.Apr 23 2020, 2:59 PM
sconstab added inline comments.
llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp
122