[PATCH] D50837: [x86/SLH] Teach SLH to harden against the "ret2spec" attack by implementing the proposed mitigation technique described in the original design document.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:2184
+  // instructions as label immediately following the call.
+  MCSymbol *RetSymbol = MF.getContext().createTempSymbol();
+  MI.setPostInstrSymbol(MF, RetSymbol);
----------------
We can give this a useful name with something like:
  MCSymbol *RetSymbol = MF.getContext().createTempSymbol("slhra", true);

Speaking of which, the second parameter to that should go away or default to true. AsmPrinter has a wrapper that does the same thing.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:2205-2208
+    // FIXME: It isn't clear that this is reliable in the face of
+    // rematerialization in the register allocator. We somehow need to force
+    // that to not occur for this particular instruction, and instead to spill
+    // or otherwise preserve the value computed *prior* to the call.
----------------
Yeah, that's a real concern... Even if we don't do it today, RA will at one point definitely want to sink this kind of LEA. You could manually do the spill yourself, and maybe mark it volatile. It adds some complexity, but this no redzone / setjmp case should be infrequent and not be performance critical code.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:2241-2242
+        .addReg(/*Index*/ 0)
+        .addImm(/*Displacement*/ -8) // The stack pointer has been popped, so
+                                     // the return address is 8-bytes past it.
+        .addReg(/*Segment*/ 0);
----------------
:)


================
Comment at: llvm/test/CodeGen/X86/speculative-load-hardening-call-and-ret.ll:105
+; X64-PIC-NEXT:    sarq $63, %rax
+; X64-PIC-NEXT:    leaq {{.*}}(%rip), %rdx
+; X64-PIC-NEXT:    cmpq %rdx, %rcx
----------------
--no_x86_scrub_rip will help make this more obviously correct.


Repository:
  rL LLVM

https://reviews.llvm.org/D50837