[X86] Add support for `-mharden-sls=[none|all|return|indirect-jmp]`

[pengfei]

Summary

The patch addresses the feature request from https://github.com/ClangBuiltLinux/linux/issues/1633. The implementation borrows a lot from aarch64.

[craig.topper]

What is #1633 in the description? Can you make a link?

[craig.topper]

llvm/lib/Target/X86/X86AsmPrinter.cpp

341

Why is TmpInst created unconditionally and not in the if?

[pengfei]

Address Craig's comment.

llvm/lib/Target/X86/X86AsmPrinter.cpp

341

Good catch!

[kristof.beyls]

See https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html, documentation for "mharden-sls": For AArch64, the options available on the command line are "retbr", "blr", "none" and "all".
I don't think the options necessarily have to be the same for x86.
But assuming I understand this patch correctly, it seems to me that with this patch -mharden-sls=all would mean fundamentally slightly different things for x86 vs arm and aarch64, which could be confusing to users.
IIUC this patch correctly, this patch implements the equivalent of aarch64/arm's -mharden-sls=retbr (i.e. add a straight-line-speculation mitigation for returns and indirect jumps, but not for indirect function calls).
Therefore, I wonder if it wouldn't be better to name this -mharden-sls=retbr for more consistency across architectures?
Or is the indirect function call case not relevant for x86 (sorry - I'm not up to speed on the details on the x86 side)?

Or does MBB.back().getDesc().isIndirectBranch() also return True for indirect calls, in which case my whole remark here can probably be ignored?

[pengfei]

Replaced isIndirectBranch with isUnconditionalBranch + isReturn.

[pengfei]

In D126137#3530777, @kristof.beyls wrote:

See https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html, documentation for "mharden-sls": For AArch64, the options available on the command line are "retbr", "blr", "none" and "all".
I don't think the options necessarily have to be the same for x86.
But assuming I understand this patch correctly, it seems to me that with this patch -mharden-sls=all would mean fundamentally slightly different things for x86 vs arm and aarch64, which could be confusing to users.
IIUC this patch correctly, this patch implements the equivalent of aarch64/arm's -mharden-sls=retbr (i.e. add a straight-line-speculation mitigation for returns and indirect jumps, but not for indirect function calls).
Therefore, I wonder if it wouldn't be better to name this -mharden-sls=retbr for more consistency across architectures?
Or is the indirect function call case not relevant for x86 (sorry - I'm not up to speed on the details on the x86 side)?

Or does MBB.back().getDesc().isIndirectBranch() also return True for indirect calls, in which case my whole remark here can probably be ignored?

Thanks for the information! I referred to the AArch64's implementation when I wrote the patch. But the ideas came from GCC. You can find the documentation of X86 on GCC https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html#x86-Options which has already been different from AArch64. The all here means 'return' + 'indirect-jmp'. So, yes, there's no indirect function call here. But it matches with GCC X86's options. (A tangential question is do we need these two options expect all? @nickdesaulniers)

[nickdesaulniers]

Thanks for the patch!

A tangential question is do we need these two options expect all?

For the Linux kernel's use, no. But I think it would extremely simple to implement. Instead of having one SubtargetFeature (FeatureHardenSlsAll), you'd have two (say FeatureHardenSlsRet and FeatureHardenSlsInd). Then the front-end decomposes -mharden-sls=all into BOTH SubtargetFeatures. You've already implemented support for either in X86AsmPrinter::emitBasicBlockEnd. You condition could instead become something like if (... && ((Subtarget->hardenSlsRet() && I->getDesc().isReturn()) || (Subtarget->hardenSlsInd() && I->getDesc().isIndirectBranch()))). You'd also need the frontend to recognize the two additional values. Do you think that's doable @pengfei ?

---

Consider the following test case:

void foo(void);

void bar(void) {
  void (*x)(void) = foo;
  x();
}

With this patch, clang -mharden-sls=all x.c -c -O2 produces:

0000000000000000 <bar>:
       0: e9 00 00 00 00               	jmp	0x5 <bar+0x5>
		0000000000000001:  R_X86_64_PLT32	foo-0x4
       5: cc                           	int3

while gcc -mharden-sls=all x.c -c -O2 produces

0000000000000000 <bar>:
       0: e9 00 00 00 00               	jmp	0x5 <bar+0x5>
		0000000000000001:  R_X86_64_PLT32	foo-0x4

So we pessimize tail calls. Please fix and add a test case for that. This might be an unintended side effect of using isUnconditionalBranch.

---

Relevant GCC commits:
c2e5c4feed32c808591b5278f680bbabe63eb225 ("x86: Generate INT3 for __builtin_eh_return")
53a643f8568067d7700a9f2facc8ba39974973d3 ("x86: Add -mharden-sls=[none|all|return|indirect-branch]")

The first seems to have some interaction between -fcf-protection and __builtin_eh_return. Is that something we need to handle?

clang/docs/ReleaseNotes.rst

371

This should be updated if additional options are supported.

You should also update clang/docs/ClangCommandLineReference.rst I think.

clang/lib/Driver/ToolChains/Arch/X86.cpp

249–250

else
        D.Diag(diag::err_invalid_sls_hardening)
            << Scope << A->getAsString(Args);

and add a test for a nonsensical value.

clang/test/Driver/x86-target-features.c

308–309

Please add a test for -mharden-sls=all -mharden-sls=none to verify that the last value "wins."

llvm/lib/Target/X86/X86AsmPrinter.cpp

342–343

Consider checking Subtarget->hardenSlsAll() first, since that's most unlikely to be unset.

if (Subtarget->hardenSlsAll()) {
  auto I = ...
}

I assume that's cheaper than finding the last non-debug instruction.

343

Should we be using MCInstrDesc::isIndirectBranch() rather than MCInstrDesc::isUnconditionalBranch()? The naming of the command line flag options seem to imply the former, not the latter.

llvm/test/CodeGen/X86/speculation-hardening-sls.ll

91

Please add a test case for the extremely simple:

void bar(void (*x)(void)) {
  x();
}

[nickdesaulniers]

In D126137#3530777, @kristof.beyls wrote:

Therefore, I wonder if it wouldn't be better to name this -mharden-sls=retbr for more consistency across architectures?

I think it's best to maintain compatibility with GCC; to do so otherwise might be surprising for users.

Or is the indirect function call case not relevant for x86 (sorry - I'm not up to speed on the details on the x86 side)?

Looks like GCC does not instrument indirect calls from what I can tell:

$ cat x.c
void bar(void (*x)(void)) {
  x();
  x();
}
$ gcc -mharden-sls=all x.c -c -O2
$ llvm-objdump -dr x.o           
...
0000000000000000 <bar>:
       0: 53                           	pushq	%rbx
       1: 48 89 fb                     	movq	%rdi, %rbx
       4: ff d7                        	callq	*%rdi
       6: 48 89 d8                     	movq	%rbx, %rax
       9: 5b                           	popq	%rbx
       a: ff e0                        	jmpq	*%rax
       c: cc                           	int3

so the indirect call instruction is not hardened. The indirect jmp (tail call) is.

[pengfei]

Address @nickdesaulniers 's comments. Thanks for the thorough review and suggestions!

So we pessimize tail calls. Please fix and add a test case for that. This might be an unintended side effect of using isUnconditionalBranch.

Done. The problem is tail calls have isReturn = 1, so we have to handle tail call specailly.

The first seems to have some interaction between -fcf-protection and __builtin_eh_return. Is that something we need to handle?

I think we should have covered the case since we do it for all JMP and RET. However, the same option doesn't generate JMP at all on Clang. So I'm not sure of that.

clang/docs/ReleaseNotes.rst

371

There's already introduction there.

[nickdesaulniers]

Diff 431607 boots for me in QEMU (triple checked that CONFIG_CC_HAS_SLS=y AND CONFIG_SLS=y were set this time ;) ).

clang/docs/ReleaseNotes.rst

371

We should expand the introduction there to state explicitly what options are supported for which targets.

clang/lib/Driver/ToolChains/Arch/X86.cpp

252–254

I think if the Scope is "none", we can simply do nothing.
@craig.topper is there a precedent for target features being "unset" like this? I guess it doesn't matter much either way...

llvm/lib/Target/X86/X86AsmPrinter.cpp

345

How come you capture Desc by reference, and I by value? Might it be simpler to just have static functions defined above X86AsmPrinter::emitBasicBlockEnd that accepts an MCInstr by reference?

346

Why isBarrier? Maybe add a comment that mentions a specific TableGen record from llvm/lib/Target/X86/X86InstrControl.td? That would help me verify that isBarrier() is correct here.

I'm guessing this is:

363 let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1,
364 isCodeGenOnly = 1, Uses = [RSP, SSP] in {

block in llvm/lib/Target/X86/X86InstrControl.td.

Otherwise llvm/lib/Target/X86/X86FrameLowering.cpp has a static function that I think is precisely what we want here, called isTailCallOpcode. Perhaps move that to a header shared between the two translation units then reuse it here? A few other backends have an IsTailCall method though they're inconsistent where they define it. It still would be good though to try to match existing conventions which makes jumping between backends easier.

347

Rather than !...isGlobal(), would it be more precise to use getOperand(0).isReg()? Looking at all of the values of enum MachineOperandType, I think it would be more precise if we check the operand is of one type, rather than "not one type."

349

So this check is logically "isn't a tail call"?

I feel like we could have a helper or lamba for "is this a tail call" and "is this an indirect call" and compose our logic here out of those two. Having descriptive names would make the code more readable, otherwise one has to think about which instruction is a return and not a call.

llvm/test/CodeGen/X86/speculation-hardening-sls.ll

8

typo

60

typo

84–95

3 typos, s/CEHCK/CHECK/g

[pengfei]

Address @nickdesaulniers 's comments. Thanks for the review!

llvm/lib/Target/X86/X86AsmPrinter.cpp

346

Why isBarrier?

Because we have conditional tail calls, see defination of TCRETURNdicc.

Perhaps move that to a header shared between the two translation units then reuse it here?

We can't. They are not overlapped.

A few other backends have an IsTailCall method though they're inconsistent where they define it

IsTailCall is an attribute of call instruction in IR. We lost it in MIR phase.

347

OK, check opcode instead.

llvm/test/CodeGen/X86/speculation-hardening-sls.ll

8

Good catch!

[nickdesaulniers]

Nice work Phoebe; thank you very much for this patch!

[MaskRay]

clang/docs/ReleaseNotes.rst

371

Delete for X86. The section is about X86.

[MaskRay]

clang/test/Driver/x86-target-features.c

308–309

-target is deprecated legacy spelling. Better to use --target=

317

The pattern does not check that SLS-IND does not have "-target-feature" "+harden-sls-ret"

[MaskRay]

clang/docs/ClangCommandLineReference.rst

368 ↗

(On Diff #432247)

This file is generated occasionally. Please don't modify it.

clang/test/Driver/x86-target-features.c

312

Since you have tested both -mharden-sls=none -mharden-sls=all and -mharden-sls=all -mharden-sls=none, single none and single all RUN lines can be deleted.

Try reducing the number of %clang RUN lines. They make testsuite slow.

[MaskRay]

clang/lib/Driver/ToolChains/Arch/X86.cpp

257

The convention is err_drv_unsupported_option_argument. Don't add a new kind.

[MaskRay]

[X86] Add support for -mharden-sls=all

The subject should list all supported values, or just say Support -mharden-sls= (don't list the values).

[MaskRay]

llvm/lib/Target/X86/X86AsmPrinter.cpp

355

This is used with basic block sections. It should be moved after int3.

[MaskRay]

gcc has renamed indirect-branch to indirect-jmp.

The rationale is that something like call *(%rbx) does not need int3.

[nickdesaulniers]

clang/docs/ClangCommandLineReference.rst

368 ↗

(On Diff #432247)

ah, it's generated from the clang/include/clang/Driver/Options.td HelpText for this option. Yeah, that should be updated.

clang/lib/Driver/ToolChains/Arch/X86.cpp

257

Disagree. Look at https://reviews.llvm.org/D81404. It's not a new diagnostic; passing an unexpected value for the equals flag is precisely what err_invalid_sls_hardening is for.

[pengfei]

Address @MaskRay 's comments. Thanks for the review!

clang/lib/Driver/ToolChains/Arch/X86.cpp

257

Yeah, I think it's OK to use it.

[nickdesaulniers]

Let's get this rebased on top of D126511.

clang/docs/ClangCommandLineReference.rst

368 ↗

(On Diff #432247)

This hunk should still be removed. @aaron.ballman confirmed to me that:

we auto-generate that
documentation when doing a sphinx build, like we do for attributes and
diagnostics: https://github.com/llvm/llvm-project/blob/d480f968ad8b56d3ee4a6b6df5532d485b0ad01e/clang/docs/CMakeLists.txt#L129

You're addition to the HelpText in clang/include/clang/Driver/Options.td is sufficient.

[pengfei]

1. Revert the change to clang/docs/ClangCommandLineReference.rst
2. Update missing options
3. Rebase on D126511

[MaskRay]

clang/docs/ReleaseNotes.rst

371

I'd add for straight-line speculation hardening

clang/include/clang/Driver/Options.td

3525 ↗

(On Diff #432692)

must be one of: .

Single quotes can be removed.

clang/lib/Driver/ToolChains/Arch/X86.cpp

255

The name harden-sls-ind seems a bit different from indirect-jmp. Use a more descriptive feature name.

clang/test/Driver/x86-target-features.c

308–309

If the feature applies to any ELF OSes, use --target=i386 (generic ELF) instead of --target=i386-unknown-linux-gnu.

313

This can cause false positive if the path components of %s have harden-sls,

...-NOT: "+harden-sls" should be more robust.

llvm/test/CodeGen/X86/speculation-hardening-sls.ll

2

Most -verify-machineinstrs usage is redundant.

[pengfei]

Address review comments. Thanks @MaskRay for the thorough review!

[nickdesaulniers]

Thanks again Phoebe for the patch!

clang/docs/ReleaseNotes.rst

 CUDA Support in Clang
 ---------------------
 
 - ...
 
 X86 Support in Clang
 --------------------
 
+- Support ``-mharden-sls=all`` for X86.
+
 DWARF Support in Clang
 ----------------------
 
 Arm and AArch64 Support in Clang
 --------------------------------
 
 Floating Point Support in Clang
 -------------------------------

clang/lib/Driver/ToolChains/Arch/X86.cpp

if (A->getOption().getID() == options::OPT_mgeneral_regs_only) {
       continue;
     }
 
     bool IsNegative = Name.startswith("no-");
     if (IsNegative)
       Name = Name.substr(3);
     Features.push_back(Args.MakeArgString((IsNegative ? "-" : "+") + Name));
   }
+
+  // Enable/disable straight line speculation hardening.
+  if (Arg *A = Args.getLastArg(options::OPT_mharden_sls_EQ)) {
+    StringRef Scope = A->getValue();
+    if (Scope == "all")
+      Features.push_back("+harden-sls-all");
+  }
 }

clang/test/Driver/x86-target-features.c

 // RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mno-avx512fp16 %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-AVX512FP16 %s
 // AVX512FP16: "-target-feature" "+avx512fp16"
 // NO-AVX512FP16: "-target-feature" "-avx512fp16"
 
 // RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mcrc32 %s -### -o %t.o 2>&1 | FileCheck -check-prefix=CRC32 %s
 // RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mno-crc32 %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-CRC32 %s
 // CRC32: "-target-feature" "+crc32"
 // NO-CRC32: "-target-feature" "-crc32"
+
+// RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mharden-sls=all %s -### -o %t.o 2>&1 | FileCheck -check-prefix=SLS %s
+// RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mharden-sls=none %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-SLS %s
+// SLS: "-target-feature" "+harden-sls-all"
+// NO-SLS-NOT: "harden-sls"

llvm/lib/Target/X86/X86.td

: SubtargetFeature<
           "into loads from being used maliciously.">;
 
 def FeatureTaggedGlobals
     : SubtargetFeature<
           "tagged-globals", "AllowTaggedGlobals", "true",
           "Use an instruction sequence for taking the address of a global "
           "that allows a memory tag in the upper address bits.">;
 
+// Control codegen mitigation against Straight Line Speculation vulnerability.
+def FeatureHardenSlsAll
+    : SubtargetFeature<
+          "harden-sls-all", "HardenSlsAll", "true",
+          "Harden against straight line speculation across RET and BR instructions.">;
+
 //===----------------------------------------------------------------------===//
 // X86 Subtarget Tuning features
 //===----------------------------------------------------------------------===//
 
 def TuningSlowSHLD : SubtargetFeature<"slow-shld", "IsSHLDSlow", "true",
                                        "SHLD instruction is slow">;
 
 def TuningSlowPMULLD : SubtargetFeature<"slow-pmulld", "IsPMULLDSlow", "true",

llvm/lib/Target/X86/X86AsmPrinter.h

public:
   const X86Subtarget &getSubtarget() const { return *Subtarget; }
 
   void emitStartOfAsmFile(Module &M) override;
 
   void emitEndOfAsmFile(Module &M) override;
 
   void emitInstruction(const MachineInstr *MI) override;
 
-  void emitBasicBlockEnd(const MachineBasicBlock &MBB) override {
-    AsmPrinter::emitBasicBlockEnd(MBB);
-    SMShadowTracker.emitShadowPadding(*OutStreamer, getSubtargetInfo());
-  }
+  void emitBasicBlockEnd(const MachineBasicBlock &MBB) override;
 
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        const char *ExtraCode, raw_ostream &O) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              const char *ExtraCode, raw_ostream &O) override;
 
   bool doInitialization(Module &M) override {
     SMShadowTracker.reset(0);

llvm/lib/Target/X86/X86AsmPrinter.cpp

if (IndexReg.getReg()) {
       unsigned ScaleVal = MI->getOperand(OpNo + X86::AddrScaleAmt).getImm();
       if (ScaleVal != 1)
         O << ',' << ScaleVal;
     }
     O << ')';
   }
 }
 
+void X86AsmPrinter::emitBasicBlockEnd(const MachineBasicBlock &MBB) {
+  AsmPrinter::emitBasicBlockEnd(MBB);
+  auto I = MBB.getLastNonDebugInstr();
+  if (I != MBB.end() && Subtarget->hardenSlsAll() &&
+      (I->getDesc().isUnconditionalBranch() || I->getDesc().isReturn())) {
+    MCInst TmpInst;
+    TmpInst.setOpcode(X86::INT3);
+    EmitToStreamer(*OutStreamer, TmpInst);
+  }
+  SMShadowTracker.emitShadowPadding(*OutStreamer, getSubtargetInfo());
+}
+
 void X86AsmPrinter::PrintMemReference(const MachineInstr *MI, unsigned OpNo,
                                       raw_ostream &O, const char *Modifier) {
   assert(isMem(*MI, OpNo) && "Invalid memory reference!");
   const MachineOperand &Segment = MI->getOperand(OpNo + X86::AddrSegmentReg);
   if (Segment.getReg()) {
     PrintModifiedOperand(MI, OpNo + X86::AddrSegmentReg, O, Modifier);
     O << ':';
   }
   PrintLeaMemReference(MI, OpNo, O, Modifier);
 }
 
 
 void X86AsmPrinter::PrintIntelMemReference(const MachineInstr *MI,

llvm/test/CodeGen/X86/speculation-hardening-sls.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mattr=harden-sls-all -verify-machineinstrs -mtriple=x86_64-unknown-unknown < %s | FileCheck %s
+
+define dso_local i32 @double_return(i32 %a, i32 %b) local_unnamed_addr {
+; CHECK-LABEL: double_return:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    testl %edi, %edi
+; CHECK-NEXT:    jle .LBB0_2
+; CHECK-NEXT:  # %bb.1: # %if.then
+; CHECK-NEXT:    movl %edi, %eax
+; CHECK-NEXT:    cltd
+; CHECK-NEXT:    idivl %esi
+; CHECK-NEXT:    retq
+; CHECK-NEXT:    int3
+; CHECK-NEXT:  .LBB0_2: # %if.else
+; CHECK-NEXT:    movl %esi, %eax
+; CHECK-NEXT:    cltd
+; CHECK-NEXT:    idivl %edi
+; CHECK-NEXT:    retq
+; CHECK-NEXT:    int3
+entry:
+  %cmp = icmp sgt i32 %a, 0
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:                                          ; preds = %entry
+  %div = sdiv i32 %a, %b
+  ret i32 %div
+
+if.else:                                          ; preds = %entry
+  %div1 = sdiv i32 %b, %a
+  ret i32 %div1
+}
+
+@__const.indirect_branch.ptr = private unnamed_addr constant [2 x i8*] [i8* blockaddress(@indirect_branch, %return), i8* blockaddress(@indirect_branch, %l2)], align 8
+
+; Function Attrs: norecurse nounwind readnone
+define dso_local i32 @indirect_branch(i32 %a, i32 %b, i32 %i) {
+; CHECK-LABEL: indirect_branch:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    movslq %edx, %rax
+; CHECK-NEXT:    jmpq *.L__const.indirect_branch.ptr(,%rax,8)
+; CHECK-NEXT:  .Ltmp0: # Block address taken
+; CHECK-NEXT:  .LBB1_2: # %return
+; CHECK-NEXT:    xorl %eax, %eax
+; CHECK-NEXT:    retq
+; CHECK-NEXT:    int3
+; CHECK-NEXT:  .Ltmp1: # Block address taken
+; CHECK-NEXT:  .LBB1_1: # %l2
+; CHECK-NEXT:    movl $1, %eax
+; CHECK-NEXT:    retq
+; CHECK-NEXT:    int3
+entry:
+  %idxprom = sext i32 %i to i64
+  %arrayidx = getelementptr inbounds [2 x i8*], [2 x i8*]* @__const.indirect_branch.ptr, i64 0, i64 %idxprom
+  %0 = load i8*, i8** %arrayidx, align 8
+  indirectbr i8* %0, [label %return, label %l2]
+
+l2:                                               ; preds = %entry
+  br label %return
+
+return:                                           ; preds = %entry, %l2
+  %retval.0 = phi i32 [ 1, %l2 ], [ 0, %entry ]
+  ret i32 %retval.0
+}
+
+define i32 @asmgoto() {
+; CHECK-LABEL: asmgoto:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    jmp .Ltmp2
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:  # %bb.1: # %asm.fallthrough
+; CHECK-NEXT:    xorl %eax, %eax
+; CHECK-NEXT:    retq
+; CHECK-NEXT:    int3
+; CHECK-NEXT:  .Ltmp2: # Block address taken
+; CHECK-NEXT:  .LBB2_2: # %d
+; CHECK-NEXT:    movl $1, %eax
+; CHECK-NEXT:    retq
+; CHECK-NEXT:    int3
+entry:
+  callbr void asm sideeffect "jmp $0", "X"(i8* blockaddress(@asmgoto, %d))
+            to label %asm.fallthrough [label %d]
+     ; The asm goto above produces a direct branch:
+
+asm.fallthrough:               ; preds = %entry
+  ret i32 0
+
+d:                             ; preds = %asm.fallthrough, %entry
+  ret i32 1
+}