Page MenuHomePhabricator

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

Authored by sconstab on Tue, Mar 10, 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.Tue, Mar 10, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Mar 10, 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].Wed, Mar 11, 12:35 PM
sconstab updated this revision to Diff 249770.Wed, Mar 11, 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.Wed, Mar 11, 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.Thu, Mar 12, 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.Thu, Mar 12, 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].Mon, Mar 16, 9:30 AM
zbrid added a comment.Tue, Mar 17, 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
78

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.Wed, Mar 18, 5:32 PM

Addressed Zola's comments.

sconstab marked 6 inline comments as done.Wed, Mar 18, 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.Thu, Apr 2, 9:38 AM