[PATCH] D75935: Add RET-hardening Support to X86 to mitigate Load Value Injection (LVI) [3/6]
Scott Constable via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 18 17:55:58 PDT 2020
sconstab marked 6 inline comments as done.
sconstab added inline comments.
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp:121
+ MachineInstr *Fence = BuildMI(MBB, MI, DebugLoc(), TII->get(X86::LFENCE));
+ addRegOffset(BuildMI(MBB, Fence, DebugLoc(), TII->get(X86::SHL64mi)),
+ X86::RSP, false, 0)
----------------
sconstab wrote:
> craig.topper wrote:
> > 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?
> 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.
Decided to keep `addRegisterDead(X86::EFLAGS, TRI)`
================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp:132
+ if (Modified)
+ ++NumFunctionsMitigated;
+ return Modified;
----------------
zbrid wrote:
> nit: Would it make sense to have this incremented when you set Modified to true instead of checking Modified here?
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75935/new/
https://reviews.llvm.org/D75935
More information about the llvm-commits
mailing list