Re: [patch 00/38] x86/retbleed: Call depth tracking mitigation

[Peter Zijlstra]

On Thu, Jul 21, 2022 at 05:54:38PM +0200, Peter Zijlstra wrote:
> My very firstest LLVM patch; except it explodes at runtime and I'm not
> sure where to start looking...
> 
> On top of sami-llvm/kcfi

Thanks Sami!

this seems to work, let me go hack the kernel..

---
 clang/lib/Driver/SanitizerArgs.cpp     | 12 ---------
 llvm/lib/Target/X86/X86AsmPrinter.cpp  | 11 --------
 llvm/lib/Target/X86/X86MCInstLower.cpp | 47 ++++++++++++++++++++++------------
 3 files changed, 31 insertions(+), 39 deletions(-)

diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index 373a74399df0..b6ebc8ad1842 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -719,18 +719,6 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
       D.Diag(diag::err_drv_argument_not_allowed_with)
           << "-fsanitize=kcfi"
           << lastArgumentForMask(D, Args, SanitizerKind::CFI);
-
-    if (Arg *A = Args.getLastArg(options::OPT_fpatchable_function_entry_EQ)) {
-      StringRef S = A->getValue();
-      unsigned N, M;
-      // With -fpatchable-function-entry=N,M, where M > 0,
-      // llvm::AsmPrinter::emitFunctionHeader injects nops before the
-      // KCFI type identifier, which is currently unsupported.
-      if (!S.consumeInteger(10, N) && S.consume_front(",") &&
-          !S.consumeInteger(10, M) && M > 0)
-        D.Diag(diag::err_drv_argument_not_allowed_with)
-            << "-fsanitize=kcfi" << A->getAsString(Args);
-    }
   }
 
   Stats = Args.hasFlag(options::OPT_fsanitize_stats,
diff --git a/llvm/lib/Target/X86/X86AsmPrinter.cpp b/llvm/lib/Target/X86/X86AsmPrinter.cpp
index 5e011d409ee8..ffdb95324da7 100644
--- a/llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ b/llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -124,23 +124,12 @@ void X86AsmPrinter::emitKCFITypeId(const MachineFunction &MF,
     OutStreamer->emitSymbolAttribute(FnSym, MCSA_ELF_TypeFunction);
   OutStreamer->emitLabel(FnSym);
 
-  // Emit int3 padding to allow runtime patching of the preamble.
-  EmitAndCountInstruction(MCInstBuilder(X86::INT3));
-  EmitAndCountInstruction(MCInstBuilder(X86::INT3));
-
   // Embed the type hash in the X86::MOV32ri instruction to avoid special
   // casing object file parsers.
   EmitAndCountInstruction(MCInstBuilder(X86::MOV32ri)
                               .addReg(X86::EAX)
                               .addImm(Type->getZExtValue()));
 
-  // The type hash is encoded in the last four bytes of the X86::MOV32ri
-  // instruction. Emit additional X86::INT3 padding to ensure the hash is
-  // at offset -6 from the function start to avoid potential call target
-  // gadgets in checks emitted by X86AsmPrinter::LowerKCFI_CHECK.
-  EmitAndCountInstruction(MCInstBuilder(X86::INT3));
-  EmitAndCountInstruction(MCInstBuilder(X86::INT3));
-
   if (MAI->hasDotTypeDotSizeDirective()) {
     MCSymbol *EndSym = OutContext.createTempSymbol("cfi_func_end");
     OutStreamer->emitLabel(EndSym);
diff --git a/llvm/lib/Target/X86/X86MCInstLower.cpp b/llvm/lib/Target/X86/X86MCInstLower.cpp
index 16c4d2e45970..4ed23348aa7c 100644
--- a/llvm/lib/Target/X86/X86MCInstLower.cpp
+++ b/llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1340,22 +1340,37 @@ void X86AsmPrinter::LowerKCFI_CHECK(const MachineInstr &MI) {
   assert(std::next(MI.getIterator())->isCall() &&
          "KCFI_CHECK not followed by a call instruction");
 
-  // The type hash is encoded in the last four bytes of the X86::CMP32mi
-  // instruction. If we decided to place the hash immediately before
-  // indirect call targets (offset -4), the X86::JCC_1 instruction we'll
-  // emit next would be a potential indirect call target as it's preceded
-  // by a valid type hash.
-  //
-  // To avoid generating useful gadgets, X86AsmPrinter::emitKCFITypeId
-  // emits the type hash prefix at offset -6, which makes X86::TRAP the
-  // only possible target in this instruction sequence.
-  EmitAndCountInstruction(MCInstBuilder(X86::CMP32mi)
-                              .addReg(MI.getOperand(0).getReg())
-                              .addImm(1)
-                              .addReg(X86::NoRegister)
-                              .addImm(-6)
-                              .addReg(X86::NoRegister)
-                              .addImm(MI.getOperand(1).getImm()));
+  const Function &F = MF->getFunction();
+  unsigned Imm = MI.getOperand(1).getImm();
+  unsigned Num = 0;
+
+  if (F.hasFnAttribute("patchable-function-prefix")) {
+    if (F.getFnAttribute("patchable-function-prefix")
+            .getValueAsString()
+            .getAsInteger(10, Num))
+      Num = 0;
+  } 
+
+  // movl $(~0x12345678), %r10d
+  EmitAndCountInstruction(MCInstBuilder(X86::MOV32ri)
+		  .addReg(X86::R10D) // dst
+		  .addImm(~Imm));
+
+  // negl %r10d
+  EmitAndCountInstruction(MCInstBuilder(X86::NEG32r)
+		  .addReg(X86::R10D) // dst
+		  .addReg(X86::R10D) // src
+		  );
+
+  // cmpl %r10d, -off(%r11)
+  EmitAndCountInstruction(MCInstBuilder(X86::CMP32mr)
+                              .addReg(MI.getOperand(0).getReg()) // base
+                              .addImm(0) // scale
+                              .addReg(0) // index
+                              .addImm(-(Num+4)) // offset
+                              .addReg(0) // segment
+                              .addReg(X86::R10D) // reg
+			      );
 
   MCSymbol *Pass = OutContext.createTempSymbol();
   EmitAndCountInstruction(