Page MenuHomePhabricator

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

Authored by pengfei on May 21 2022, 8:57 PM.

Details

Summary

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 21 2022, 8:57 PM
pengfei edited the summary of this revision. (Show Details)May 21 2022, 8:57 PM

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

craig.topper added inline comments.May 21 2022, 9:01 PM
llvm/lib/Target/X86/X86AsmPrinter.cpp
341

Why is TmpInst created unconditionally and not in the if?

pengfei edited the summary of this revision. (Show Details)May 21 2022, 9:01 PM
pengfei updated this revision to Diff 431193.May 21 2022, 9:03 PM

Address Craig's comment.

llvm/lib/Target/X86/X86AsmPrinter.cpp
341

Good catch!

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 updated this revision to Diff 431322.May 23 2022, 3:58 AM

Replaced isIndirectBranch with isUnconditionalBranch + isReturn.

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)

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
452

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
253–254
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
92

Please add a test case for the extremely simple:

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

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 updated this revision to Diff 431607.May 24 2022, 12:58 AM
pengfei marked 5 inline comments as done.

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
452

There's already introduction there.

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
452

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

clang/lib/Driver/ToolChains/Arch/X86.cpp
256–258

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 updated this revision to Diff 432247.May 26 2022, 5:02 AM
pengfei marked 6 inline comments as done.

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 accepted this revision.May 26 2022, 11:21 AM

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

This revision is now accepted and ready to land.May 26 2022, 11:21 AM
MaskRay added inline comments.May 26 2022, 1:25 PM
clang/docs/ReleaseNotes.rst
452

Delete for X86. The section is about X86.

MaskRay added inline comments.May 26 2022, 1:28 PM
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 added inline comments.May 26 2022, 1:55 PM
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 requested changes to this revision.May 26 2022, 1:57 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChains/Arch/X86.cpp
261

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

This revision now requires changes to proceed.May 26 2022, 1:57 PM

[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 added inline comments.May 26 2022, 2:21 PM
llvm/lib/Target/X86/X86AsmPrinter.cpp
355

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

gcc has renamed indirect-branch to indirect-jmp.

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

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
261

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 updated this revision to Diff 432511.May 27 2022, 3:21 AM
pengfei marked 5 inline comments as done.

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

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

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

pengfei retitled this revision from [X86] Add support for `-mharden-sls=all` to [X86] Add support for `-mharden-sls=[none|all|return|indirect-jmp]`.May 27 2022, 3:21 AM
nickdesaulniers requested changes to this revision.May 27 2022, 10:30 AM
nickdesaulniers added a subscriber: aaron.ballman.

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.

This revision now requires changes to proceed.May 27 2022, 10:30 AM
pengfei updated this revision to Diff 432692.May 27 2022, 8:51 PM
  1. Revert the change to clang/docs/ClangCommandLineReference.rst
  2. Update missing options
  3. Rebase on D126511
MaskRay added inline comments.May 27 2022, 10:39 PM
clang/docs/ReleaseNotes.rst
452

I'd add for straight-line speculation hardening

clang/include/clang/Driver/Options.td
3529

must be one of: .

Single quotes can be removed.

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

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 updated this revision to Diff 432698.May 27 2022, 11:10 PM
pengfei marked 6 inline comments as done.

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

MaskRay accepted this revision.May 28 2022, 9:53 PM
nickdesaulniers accepted this revision.May 31 2022, 9:48 AM

Thanks again Phoebe for the patch!

This revision is now accepted and ready to land.May 31 2022, 9:48 AM