[PATCH] D75934: Add Indirect Thunk Support to X86 to mitigate Load Value Injection (LVI) [2/6]

[Zola Bridges via Phabricator via llvm-commits]

zbrid added a comment.

Looks great! Thanks for writing this! I had a bunch of nits (sorry!) and a few questions, otherwise LGTM. Please wait for sign off from at least one other person before submitting.



================
Comment at: llvm/lib/Target/X86/X86FastISel.cpp:3210
 
   // Functions using retpoline for indirect calls need to use SDISel.
+  if (Subtarget->useRetpolineIndirectCalls() ||
----------------
nit: Update comment?


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:966
+  if (Is64Bit && IsLargeCodeModel && (STI.useRetpolineIndirectCalls() ||
+                                      STI.useLVIControlFlowIntegrity()))
     report_fatal_error("Emitting stack probe calls on 64-bit with the large "
----------------
Would it make sense to add a separate check for LVI-CFI and have a corresponding error message referencing specifically LVI-CFI rather than retpolines?

---

I wrote this in several spots, sorry about the repetition.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2707
     // FIXME: Add retpoline support and remove the error here..
-    if (STI.useRetpolineIndirectCalls())
+    if (STI.useRetpolineIndirectCalls() || STI.useLVIControlFlowIntegrity())
       report_fatal_error("Emitting morestack calls on 64-bit with the large "
----------------
Would it make sense to add a separate check for LVI-CFI and have a corresponding error message referencing specifically LVI-CFI rather than retpolines?




================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30557
 bool X86TargetLowering::areJTsAllowed(const Function *Fn) const {
   // If the subtarget is using retpolines, we need to not generate jump tables.
+  if (Subtarget.useRetpolineIndirectBranches() ||
----------------
nit: Update comment to mention LVI-CFI


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31844
 MachineBasicBlock *
 X86TargetLowering::EmitLoweredRetpoline(MachineInstr &MI,
                                         MachineBasicBlock *BB) const {
----------------
Can you change this function name? If I'm understanding this code correctly, this no longer only applies to retpoline, but also to LVI thunks, so the naming is misleading.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:79
+
+bool X86LoadValueInjectionIndirectThunksPass::doInitialization(Module &M) {
+  InsertedThunks = false;
----------------
I want to make sure I understand this correctly: You use this function to initialize InsertedThunks so that, for each module, InsertedThunks is shared across all the functions. This enables the Module to ensure the thunk is only inserted once. Is that right?


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:87
+  STI = &MF.getSubtarget<X86Subtarget>();
+  if (!(STI->hasSSE2() || STI->is64Bit())) {
+    // FIXME: support 32-bit
----------------
Why is 32-bit okay if it has SSE2 features? (Asking to understand since my processor knowledge is a bit weak. Thanks!)


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:92
+
+  // Don't skip functions with the "optnone" attr but participate in opt-bisect.
+  const Function &F = MF.getFunction();
----------------
Why did you decide to make this not skip functions with optnone?


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:98
+
+  LLVM_DEBUG(dbgs() << "***** " << getPassName() << " : " << MF.getName()
+                    << " *****\n");
----------------
I think this debugging message should be at the top of the function.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:129
+
+  assert(MF.getName() == "__x86_indirect_thunk_r11" &&
+         "Should only have an r11 thunk on 64-bit targets");
----------------
Should this use R11ThunkName instead of this string literal?


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:149
+  // inline.
+  AttrBuilder B;
+  B.addAttribute(llvm::Attribute::NoUnwind);
----------------
I see this list is from the retpoline pass. I don't know what each of these things do, but just wondering if you double checked these are the same attributes we want for this thunk?


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1227
+      if (Subtarget->useRetpolineIndirectCalls() ||
+          Subtarget->useLVIControlFlowIntegrity())
         report_fatal_error("Lowering register statepoints with retpoline not "
----------------
Would it make sense to add a separate check for LVI-CFI and have a corresponding error message referencing specifically LVI-CFI rather than retpolines?




================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1407
+    if (Subtarget->useRetpolineIndirectCalls() ||
+        Subtarget->useLVIControlFlowIntegrity())
       report_fatal_error(
----------------
Would it make sense to add a separate check for LVI-CFI and have a corresponding error message referencing specifically LVI-CFI rather than retpolines?


================
Comment at: llvm/test/CodeGen/X86/O0-pipeline.ll:76
 ; CHECK-NEXT:       X86 Retpoline Thunks
+; CHECK-NEXT:       X86 Load Value Injection (LVI) Indirect Thunks Pass
 ; CHECK-NEXT:       Check CFA info and insert CFI instructions if needed
----------------
nit: remove pass to follow what most of the other passes do here (requires change in pass code and here)


================
Comment at: llvm/test/CodeGen/X86/O3-pipeline.ll:185
 ; CHECK-NEXT:       X86 Retpoline Thunks
+; CHECK-NEXT:       X86 Load Value Injection (LVI) Indirect Thunks Pass
 ; CHECK-NEXT:       Check CFA info and insert CFI instructions if needed
----------------
Same comment as in O0-pipeline.ll test


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

https://reviews.llvm.org/D75934