The patch addresses the feature request from https://github.com/ClangBuiltLinux/linux/issues/1633. The implementation borrows a lot from aarch64.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/X86/X86AsmPrinter.cpp | ||
---|---|---|
341 | Why is TmpInst created unconditionally and not in the if? |
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(); } |
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.
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. | |
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, 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 |
Address @nickdesaulniers 's comments. Thanks for the review!
llvm/lib/Target/X86/X86AsmPrinter.cpp | ||
---|---|---|
346 |
Because we have conditional tail calls, see defination of TCRETURNdicc.
We can't. They are not overlapped.
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! |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
452 | Delete for X86. The section is about X86. |
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. |
clang/lib/Driver/ToolChains/Arch/X86.cpp | ||
---|---|---|
261 | The convention is err_drv_unsupported_option_argument. Don't add a new kind. |
[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).
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. |
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:
You're addition to the HelpText in clang/include/clang/Driver/Options.td is sufficient. |
- Revert the change to clang/docs/ClangCommandLineReference.rst
- Update missing options
- Rebase on D126511
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. |
This should be updated if additional options are supported.
You should also update clang/docs/ClangCommandLineReference.rst I think.