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

Peter Zijlstra
2022-07-21T17:56:40Z

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(