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

[Scott Constable via Phabricator via llvm-commits]

sconstab marked 3 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)
----------------
mattdr wrote:
> sconstab wrote:
> > 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)`
> 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.
Thanks! Will fix.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionRetHardening.cpp:82-83
+
+  // We can clobber any register allowed by the function's calling convention.
+  for (const MCPhysReg *PR = TRI->getCalleeSavedRegs(&MF); auto Reg = *PR; ++PR)
+    UnclobberableGR64s.set(Reg);
----------------
mattdr wrote:
> 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.
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75935/new/

https://reviews.llvm.org/D75935