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

[Craig Topper via Phabricator via cfe-commits]

craig.topper abandoned this revision.
craig.topper added a comment.

This was superceded by D76812 


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

https://reviews.llvm.org/D75934



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Matthew Riley via Phabricator via cfe-commits]

mattdr added a comment.

I accidentally spent time reviewing this, only to cross-reference something in 
the LLVM code and find another diff (https://reviews.llvm.org/D76812) was 
written to answer Zola's request and has already been submitted.


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

https://reviews.llvm.org/D75934



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zola Bridges via Phabricator via cfe-commits]

zbrid added a comment.

I followed up with Chandler about whether it would make sense to integrate this 
with the existing retpolines pass as you and Craig suggested. He supported the 
idea. Could you create a new patch(es) to do the refactor/renaming of the 
retpolines thunking pass and instruction scheduling conditionals to be more 
general and then add the LVI option?


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

https://reviews.llvm.org/D75934



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 ) {
+  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 = ();
+  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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Scott Constable via Phabricator via cfe-commits]

sconstab updated this revision to Diff 251193.
sconstab added a comment.

Addressed some of Zola's comments, and removed some unnecessary assertions.


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

https://reviews.llvm.org/D75934

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Arch/X86.cpp
  llvm/lib/Target/X86/CMakeLists.txt
  llvm/lib/Target/X86/X86.h
  llvm/lib/Target/X86/X86.td
  llvm/lib/Target/X86/X86FastISel.cpp
  llvm/lib/Target/X86/X86FrameLowering.cpp
  llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86ISelLowering.h
  llvm/lib/Target/X86/X86InstrCompiler.td
  llvm/lib/Target/X86/X86InstrControl.td
  llvm/lib/Target/X86/X86InstrInfo.td
  llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp
  llvm/lib/Target/X86/X86MCInstLower.cpp
  llvm/lib/Target/X86/X86Subtarget.h
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/CodeGen/X86/O0-pipeline.ll
  llvm/test/CodeGen/X86/O3-pipeline.ll
  llvm/test/CodeGen/X86/lvi-hardening-indirectbr.ll

Index: llvm/test/CodeGen/X86/lvi-hardening-indirectbr.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/lvi-hardening-indirectbr.ll
@@ -0,0 +1,282 @@
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-unknown < %s | FileCheck %s --check-prefix=X64
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-unknown -O0 < %s | FileCheck %s --check-prefix=X64FAST
+;
+; Note that a lot of this code was lifted from retpoline.ll.
+
+declare void @bar(i32)
+
+; Test a simple indirect call and tail call.
+define void @icall_reg(void (i32)* %fp, i32 %x) #0 {
+entry:
+  tail call void @bar(i32 %x)
+  tail call void %fp(i32 %x)
+  tail call void @bar(i32 %x)
+  tail call void %fp(i32 %x)
+  ret void
+}
+
+; X64-LABEL: icall_reg:
+; X64-DAG:   movq %rdi, %[[fp:[^ ]*]]
+; X64-DAG:   movl %esi, %[[x:[^ ]*]]
+; X64:   movl %esi, %edi
+; X64:   callq bar
+; X64-DAG:   movl %[[x]], %edi
+; X64-DAG:   movq %[[fp]], %r11
+; X64:   callq __x86_indirect_thunk_r11
+; X64:   movl %[[x]], %edi
+; X64:   callq bar
+; X64-DAG:   movl %[[x]], %edi
+; X64-DAG:   movq %[[fp]], %r11
+; X64:   jmp __x86_indirect_thunk_r11 # TAILCALL
+
+; X64FAST-LABEL: icall_reg:
+; X64FAST:   callq bar
+; X64FAST:   callq __x86_indirect_thunk_r11
+; X64FAST:   callq bar
+; X64FAST:   jmp __x86_indirect_thunk_r11 # TAILCALL
+
+
+@global_fp = external global void (i32)*
+
+; Test an indirect call through a global variable.
+define void @icall_global_fp(i32 %x, void (i32)** %fpp) #0 {
+  %fp1 = load void (i32)*, void (i32)** @global_fp
+  call void %fp1(i32 %x)
+  %fp2 = load void (i32)*, void (i32)** @global_fp
+  tail call void %fp2(i32 %x)
+  ret void
+}
+
+; X64-LABEL: icall_global_fp:
+; X64-DAG:   movl %edi, %[[x:[^ ]*]]
+; X64-DAG:   movq global_fp(%rip), %r11
+; X64:   callq __x86_indirect_thunk_r11
+; X64-DAG:   movl %[[x]], %edi
+; X64-DAG:   movq global_fp(%rip), %r11
+; X64:   jmp __x86_indirect_thunk_r11 # TAILCALL
+
+; X64FAST-LABEL: icall_global_fp:
+; X64FAST:   movq global_fp(%rip), %r11
+; X64FAST:   callq __x86_indirect_thunk_r11
+; X64FAST:   movq global_fp(%rip), %r11
+; X64FAST:   jmp __x86_indirect_thunk_r11 # TAILCALL
+
+
+%struct.Foo = type { void (%struct.Foo*)** }
+
+; Test an indirect call through a vtable.
+define void @vcall(%struct.Foo* %obj) #0 {
+  %vptr_field = getelementptr %struct.Foo, %struct.Foo* %obj, i32 0, i32 0
+  %vptr = load void (%struct.Foo*)**, void (%struct.Foo*)*** %vptr_field
+  %vslot = getelementptr void(%struct.Foo*)*, void(%struct.Foo*)** %vptr, i32 1
+  %fp = load void(%struct.Foo*)*, void(%struct.Foo*)** %vslot
+  tail call void %fp(%struct.Foo* %obj)
+  tail call void %fp(%struct.Foo* %obj)
+  ret void
+}
+
+; X64-LABEL: vcall:
+; X64:   movq %rdi, %[[obj:[^ ]*]]
+; X64:   movq (%rdi), %[[vptr:[^ ]*]]
+; X64:   movq 8(%[[vptr]]), %[[fp:[^ ]*]]
+; X64:   movq %[[fp]], %r11
+; X64:   callq __x86_indirect_thunk_r11
+; X64-DAG:   movq %[[obj]], %rdi
+; X64-DAG:   movq %[[fp]], %r11
+; X64:   jmp __x86_indirect_thunk_r11 # TAILCALL
+
+; X64FAST-LABEL: vcall:
+; X64FAST:   callq __x86_indirect_thunk_r11
+; X64FAST:   jmp __x86_indirect_thunk_r11 # TAILCALL
+
+
+declare void @direct_callee()
+
+define void @direct_tail() #0 {
+  tail call void @direct_callee()
+  ret void
+}
+
+; X64-LABEL: direct_tail:
+; X64:   jmp direct_callee # TAILCALL
+; X64FAST-LABEL: direct_tail:
+; X64FAST:   jmp direct_callee # TAILCALL
+
+
+declare void @nonlazybind_callee() #1
+
+define void @nonlazybind_caller() #0 {
+  call void @nonlazybind_callee()
+  tail call void @nonlazybind_callee()
+  ret void
+}
+
+; X64-LABEL: nonlazybind_caller:
+; X64:   movq nonlazybind_callee@GOTPCREL(%rip), %[[REG:.*]]
+; X64:   movq %[[REG]], %r11
+; X64:   callq 

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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp:92
+
+  // Don't skip functions with the "optnone" attr but participate in 
opt-bisect.
+  const Function  = MF.getFunction();

zbrid wrote:
> Why did you decide to make this not skip functions with optnone?
This suggestion came from myself and Andy Kaylor. skipFunction checks 
opt-bisect-limit and the optnone attribute. This would make the pass not run at 
-O0, but that seemed bad. The explicit optnone check here was to avoid the 
optnone behavior in skipFunction, but make opt-bisect-limit work. But looking 
this now, I think this left us in a situation where opt-bisect-limit doesn't 
work for this pass on optnone functions.


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

https://reviews.llvm.org/D75934



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zola Bridges via Phabricator via cfe-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 ,
 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 ) {
+  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 = ();
+  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  = 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