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

[Scott Constable via Phabricator via cfe-commits]

sconstab marked 12 inline comments as done.
sconstab added inline comments.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:79
+
+bool X86LoadValueInjectionIndirectThunksPass::doInitialization(Module &M) {
+  InsertedThunks = false;
----------------
zbrid wrote:
> 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?
As mentioned in a code comment at the beginning of this file, a lot of this code was lifted from X86RetpolineThunks.cpp, including the logic to insert one thunk per module. So I didn't actually compose this logic, but my understanding of it seems to match yours.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:87
+  STI = &MF.getSubtarget<X86Subtarget>();
+  if (!(STI->hasSSE2() || STI->is64Bit())) {
+    // FIXME: support 32-bit
----------------
zbrid wrote:
> Why is 32-bit okay if it has SSE2 features? (Asking to understand since my processor knowledge is a bit weak. Thanks!)
Actually it isn't okay (at this time), so you were correct to point this out. The thunks right now clearly only support 64-bit. It may be conceivable to support 32-bit that also has SSE2, but I think this would require some extra work.


================
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");
----------------
zbrid wrote:
> Should this use R11ThunkName instead of this string literal?
I just removed the assertion because it wasn't really necessary.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:149
+  // inline.
+  AttrBuilder B;
+  B.addAttribute(llvm::Attribute::NoUnwind);
----------------
zbrid wrote:
> 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?
I do think that these are correct. From the LLVM reference manual, NoUnwind means that "This function attribute indicates that the function never raises an exception," which should hold for the thunk. The Naked attribute "disables prologue / epilogue emission for the function," which is something we would not want.


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

https://reviews.llvm.org/D75934