[X86] Automatically harden inline assembly RET instructions against Load Value Injection (LVI)

[sconstab]

Summary

Previously, the X86AsmParser would issue a warning whenever a ret instruction is encountered. This patch changes the behavior to automatically transform each ret instruction in an inline assembly stream into:

shlq $0, (%rsp)
lfence
ret

which is secure, according to https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection#specialinstructions.

[craig.topper]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3215

Why did emitInstruction have to get repeated everywhere? Aren't we still emitting the original ret from the user after the mitigation? So its not any different than inserting an LFENCE. Am I missing something?

[sconstab]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3215

It's a little more complicated now because the ret mitigation requires the LFENCE to be inserted *before* ret, whereas all other loading instructions need the LFENCE after. But maybe there is a cleaner way to implement this that I am missing?

[craig.topper]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3215

right the others are after. why can't i ever remember that.

What if we called applyLVICFIMitigation before the emitInstruction and called applyLVILoadHardeningMitigation after?

[sconstab]

Addressed suggestion by @craig.topper to consolidate calls to emitInstruction().

[craig.topper]

LGTM

[mattdr]

LGTM, one question about source locations

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3199

Seems like this will have implications for debugging. Can we copy the SMLoc from the original instruction, for this and for the other MCInsts we're creating?

[craig.topper]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3199

You mean like debugging in GDB?

SMLoc is just the pointer into the memory buffer that holds the assembly text. It has no effect on debug locations in the final binary. It's main use is to print diagnostics in the compiler.

[mattdr]

Ack, thanks for setting me straight on that. Yes, I was talking about the mapping of assembly instructions back to sourceloc that could be displayed in GDB.

[craig.topper]

Committed in 0cbe713c69f7c12682855ffd61ecb0d7edc262e3

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

private:
 
   unsigned checkTargetMatchPredicate(MCInst &Inst) override;
 
   bool validateInstruction(MCInst &Inst, const OperandVector &Ops);
   bool processInstruction(MCInst &Inst, const OperandVector &Ops);
 
   // Load Value Injection (LVI) Mitigations for machine code
   void emitWarningForSpecialLVIInstruction(SMLoc Loc);
-  bool applyLVICFIMitigation(MCInst &Inst);
+  bool applyLVICFIMitigation(MCInst &Inst, MCStreamer &Out);
   bool applyLVILoadHardeningMitigation(MCInst &Inst, MCStreamer &Out);
 
   /// Wrapper around MCStreamer::emitInstruction(). Possibly adds
   /// instrumentation around Inst.
   void emitInstruction(MCInst &Inst, OperandVector &Operands, MCStreamer &Out);
 
   bool MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                OperandVector &Operands, MCStreamer &Out,

 /// combine a load and a branch within a single instruction. To mitigate these
 /// instructions against LVI, they must be decomposed into separate load and
 /// branch instructions, with an LFENCE in between. For more details, see:
 /// - X86LoadValueInjectionRetHardening.cpp
 /// - X86LoadValueInjectionIndirectThunks.cpp
 /// - https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection
 ///
 /// Returns `true` if a mitigation was applied or warning was emitted.
-bool X86AsmParser::applyLVICFIMitigation(MCInst &Inst) {
+bool X86AsmParser::applyLVICFIMitigation(MCInst &Inst, MCStreamer &Out) {
   // Information on control-flow instructions that require manual mitigation can
   // be found here:
   // https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection#specialinstructions
   switch (Inst.getOpcode()) {
   case X86::RETW:
   case X86::RETL:
   case X86::RETQ:
   case X86::RETIL:
   case X86::RETIQ:
-  case X86::RETIW:
+  case X86::RETIW: {
+    MCInst ShlInst, FenceInst;
+    bool Parse32 = is32BitMode() || Code16GCC;
+    unsigned Basereg =
+        is64BitMode() ? X86::RSP : (Parse32 ? X86::ESP : X86::SP);
+    const MCExpr *Disp = MCConstantExpr::create(0, getContext());
+    auto ShlMemOp = X86Operand::CreateMem(getPointerWidth(), /*SegReg=*/0, Disp,
+                                          /*BaseReg=*/Basereg, /*IndexReg=*/0,
+                                          /*Scale=*/1, SMLoc{}, SMLoc{}, 0);
+    ShlInst.setOpcode(X86::SHL64mi);
+    ShlMemOp->addMemOperands(ShlInst, 5);
+    ShlInst.addOperand(MCOperand::createImm(0));
+    FenceInst.setOpcode(X86::LFENCE);
+    Out.emitInstruction(ShlInst, getSTI());
+    Out.emitInstruction(FenceInst, getSTI());
+    return true;
+  }
   case X86::JMP16m:
   case X86::JMP32m:
   case X86::JMP64m:
   case X86::CALL16m:
   case X86::CALL32m:
   case X86::CALL64m:
     emitWarningForSpecialLVIInstruction(Inst.getLoc());
     return true;
   }
   return false;
 }
 
 /// To mitigate LVI, every instruction that performs a load can be followed by
 /// an LFENCE instruction to squash any potential mis-speculation. There are
 /// some instructions that require additional considerations, and may requre
 /// manual mitigation. For more details, see:
if ((Flags & X86::IP_HAS_REPEAT) || (Flags & X86::IP_HAS_REPEAT_NE)) {
     return true;
   }
 
   const MCInstrDesc &MCID = MII.get(Inst.getOpcode());
   // LFENCE has the mayLoad property, don't double fence.
   if (MCID.mayLoad() && Inst.getOpcode() != X86::LFENCE) {
     MCInst FenceInst;
     FenceInst.setOpcode(X86::LFENCE);
-    FenceInst.setLoc(Inst.getLoc());
     Out.emitInstruction(FenceInst, getSTI());
     return true;
   }
   return false;
 }
 
 void X86AsmParser::emitInstruction(MCInst &Inst, OperandVector &Operands,
                                    MCStreamer &Out) {
-  Out.emitInstruction(Inst, getSTI());
-
+  bool MitigatedLVICFI = false;
   if (LVIInlineAsmHardening) {
-    if (getSTI().getFeatureBits()[X86::FeatureLVIControlFlowIntegrity] &&
-        applyLVICFIMitigation(Inst))
-      return;
+    if (getSTI().getFeatureBits()[X86::FeatureLVIControlFlowIntegrity])
+      MitigatedLVICFI = applyLVICFIMitigation(Inst, Out);
+  }
+  Out.emitInstruction(Inst, getSTI());
+  if (LVIInlineAsmHardening && !MitigatedLVICFI) {
     if (getSTI().getFeatureBits()[X86::FeatureLVILoadHardening])
       applyLVILoadHardeningMitigation(Inst, Out);
   }
 }
 
 bool X86AsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                            OperandVector &Operands,
                                            MCStreamer &Out, uint64_t &ErrorInfo,

llvm/test/CodeGen/X86/lvi-hardening-inline-asm.ll

 ; RUN: llc -verify-machineinstrs -mtriple=x86_64-unknown -mattr=+lvi-load-hardening -mattr=+lvi-cfi -x86-experimental-lvi-inline-asm-hardening < %s -o %t.out 2> %t.err
 ; RUN: FileCheck %s --check-prefix=X86 < %t.out
 ; RUN: FileCheck %s --check-prefix=WARN < %t.err
 
 ; Test module-level assembly
 module asm "pop %rbx"
 module asm "ret"
-; WARN:      warning: Instruction may be vulnerable to LVI
-; WARN-NEXT: ret
-; WARN-NEXT: ^
-; WARN-NEXT: note: See https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection#specialinstructions for more information
+; X86:      popq %rbx
+; X86-NEXT: lfence
+; X86-NEXT: shlq $0, (%rsp)
+; X86-NEXT: lfence
+; X86-NEXT: retq
 
 ; Function Attrs: noinline nounwind optnone uwtable
 define dso_local void @test_inline_asm() {
 entry:
 ; X86-LABEL: test_inline_asm:
   call void asm sideeffect "mov 0x3fed(%rip),%rax", "~{dirflag},~{fpsr},~{flags}"() #1
 ; X86:      movq  16365(%rip), %rax
 ; X86-NEXT: lfence

 ; WARN:      warning: Instruction may be vulnerable to LVI
 ; WARN-NEXT: repnz
 ; WARN-NEXT: ^
 ; WARN-NEXT: note: See https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection#specialinstructions for more information
   call void asm sideeffect "pinsrw $$0x6,(%eax),%xmm0", "~{dirflag},~{fpsr},~{flags}"() #1
 ; X86:      pinsrw  $6, (%eax), %xmm0
 ; X86-NEXT: lfence
   call void asm sideeffect "ret", "~{dirflag},~{fpsr},~{flags}"() #1
-; WARN:      warning: Instruction may be vulnerable to LVI
-; WARN-NEXT: ret
-; WARN-NEXT: ^
-; WARN-NEXT: note: See https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection#specialinstructions for more information
-; X86:      retq
+; X86:      shlq $0, (%rsp)
+; X86-NEXT: lfence
+; X86-NEXT: retq
 ; X86-NOT:  lfence
   call void asm sideeffect "ret $$8", "~{dirflag},~{fpsr},~{flags}"() #1
-; WARN:      warning: Instruction may be vulnerable to LVI
-; WARN-NEXT: ret $8
-; WARN-NEXT: ^
-; WARN-NEXT: note: See https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection#specialinstructions for more information
-; X86:      retq  $8
+; X86:      shlq $0, (%rsp)
+; X86-NEXT: lfence
+; X86-NEXT: retq $8
 ; X86-NOT:  lfence
   call void asm sideeffect "jmpq *(%rdx)", "~{dirflag},~{fpsr},~{flags}"() #1
 ; WARN:      warning: Instruction may be vulnerable to LVI
 ; WARN-NEXT: jmpq *(%rdx)
 ; WARN-NEXT: ^
 ; WARN-NEXT: note: See https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection#specialinstructions for more information
 ; X86:      jmpq *(%rdx)
 ; X86-NOT:  lfence