Page MenuHomePhabricator

Support -fstack-clash-protection for x86
ClosedPublic

Authored by serge-sans-paille on Oct 9 2019, 11:30 AM.

Details

Summary

Implement protection against the stack clash attack [0] through inline stack probing.

Probe stack allocation every PAGE_SIZE during frame lowering or dynamic allocation to make sure the page guard, if any, is touched when touching the stack, in a similar manner to GCC[1].

This extends the existing probe-stack mechanism with a special value inline-asm. Technically the former uses function call before stack allocation while this patch provides inlined stack probes and chunk allocation.

Only implemented for x86.

[0] https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt
[1] https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00556.html

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@sylvestre.ledru did the testing and benchmarking on firefox (see https://bugzilla.mozilla.org/show_bug.cgi?id=1588710#c12), everything seems ok, let's move forward?

@sylvestre.ledru did the testing and benchmarking on firefox (see https://bugzilla.mozilla.org/show_bug.cgi?id=1588710#c12), everything seems ok, let's move forward?

Actually, not sure about the benchmark anymore. I am checking with experts. Sorry

Added extra test for arch support (warns if the flag is used on unsupported targets, and checks it's effectively unused), cc @sylvestre.ledru

No, still waiting for @rnk feedback. This commit is just me using the wrong remote when pushing for validation.

For the record : this validates fine on OSX, Linux and Windows, using the github actions setup by @tstellar

https://github.com/serge-sans-paille/llvm-project/pull/3

@rnk up? the mozilla folks confirmed there's no significant regression (see https://bugzilla.mozilla.org/show_bug.cgi?id=1588710#c12).

craig.topper added inline comments.Jan 6 2020, 10:48 AM
clang/docs/ClangCommandLineReference.rst
1914

Probably need a -fno-stack-class-protection as well. Looks like gcc has it. You'll need to update the handling in the driver to make sure the last polarity wins. For cc1 you might be able to support just the positive variant. But try to see what we usually do.

clang/lib/Basic/Targets/X86.h
169

What about 32-bit mode where the register name is "esp"?

clang/lib/CodeGen/CGStmt.cpp
2255

Why is this in the frontend diagnostic list?

clang/lib/Driver/ToolChains/Clang.cpp
2999

Can we use EffectiveTriple.isX86() that was just introduced yesterday?

3008

Seems like this should just be an if? Or maybe use Args.filtered or args getLastArg?

tstellar added inline comments.Jan 9 2020, 1:05 PM
clang/lib/Driver/ToolChains/Clang.cpp
3001

Do we need to warn (or error) here for arches that don't implement -fstack-clash-protection?

Take review into account.

serge-sans-paille marked 9 inline comments as done.Jan 10 2020, 6:03 AM
serge-sans-paille added inline comments.
clang/lib/CodeGen/CGStmt.cpp
2255

As far as I understand, that's the only place where line/col accurate diagnostics are emitted.

clang/lib/Driver/ToolChains/Clang.cpp
3001

That's done indirectly because the argument is not claimed on unsupported platform.

@craig.topper do you think there is a chance that this change could be part of clang-10 ?
Thanks

craig.topper added inline comments.Jan 10 2020, 8:45 AM
clang/lib/CodeGen/CGStmt.cpp
2255

I was really asking about why we have to use FrontendDiagnostic.h and DriverDiagnostic.h? Is that what we normally use for diagnostics in Codegen files? Seems like a possible layering issue for modules build

serge-sans-paille marked 2 inline comments as done.Jan 10 2020, 9:50 AM
serge-sans-paille added inline comments.
clang/lib/CodeGen/CGStmt.cpp
2255

Oh, I understand now, let me have a look, your remark totally makes sense.

  • update warn_fe_stack_clash_protection_inline_asm location.
serge-sans-paille marked an inline comment as done.Jan 11 2020, 12:09 AM
craig.topper added inline comments.Jan 16 2020, 11:03 AM
clang/include/clang/Basic/DiagnosticCommonKinds.td
243

Remove "fe_" from this? Looks like thats only there because this used to be DiagnosticFrontendKinds?

clang/lib/Basic/Targets/X86.h
169

This probably doesn't work correctly with X32 which is 64-bit but uses 32-bit pointers. Maybe we change this to "bool isSPReg(StringRef Reg)" and pass the register name to it so you can just check both strings always.

clang/lib/Driver/ToolChains/Clang.cpp
3002

I think this can use Args.hasFlag(options::OPT_fstack_clash_protection, options::OPT_fnostack_clash_protection, false) and then you don't need the 'if'

llvm/lib/Target/X86/X86ISelLowering.cpp
31173

This uses physSPReg, but doesn't match the condition for when physSPReg is a 64-bit register. Same at several places below.

llvm/lib/Target/X86/X86InstrCompiler.td
130

Why is this In64BitMode, but above is NotLP64. Shouldn't they be opposites? Looks like this was just copied from SEG_ALLOCA above?

serge-sans-paille marked 5 inline comments as done.

Take into account some of the reviews

serge-sans-paille marked 2 inline comments as done.Jan 17 2020, 5:44 AM
serge-sans-paille added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
3002

I wasn't aware of that API, thanks for pointing this out o/

llvm/lib/Target/X86/X86ISelLowering.cpp
31173

Sorry, I don't understand your remark. Can you elaborate?

llvm/lib/Target/X86/X86InstrCompiler.td
130

Yeah, these intrinsics are just the same as the one above, except they have a different name. And they get lowered differently. I'm not familiar with that part of LLVM, can you suggest a better approach?

Unit tests: fail. 61960 tests passed, 3 failed and 783 were skipped.

failed: Clang.CodeGen/stack-clash-protection.c
failed: Clang.Driver/stack-clash-protection.c
failed: Clang.Misc/warning-flags.c

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

craig.topper added inline comments.Jan 17 2020, 10:31 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
31173

If physSPReg is RSP then you need to use SUB64ri32 and if its ESP you need to use SUB32ri. The condition you're using here is "IsLP64", but the condition for phySPReg to be RSP is "IsLP64 || Subtarget.isTargetNaCl64()". So there's a mismatch when IsLP64 is false and Subtarget.isTargetNaCl64() is true.

  • Harmonize esp/rsp usage as hinted by @craig.topper
  • Fix argument detection

Unit tests: fail. 62035 tests passed, 2 failed and 783 were skipped.

failed: Clang.CodeGen/stack-clash-protection.c
failed: Clang.Misc/warning-flags.c

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Update warning category

Unit tests: fail. 62059 tests passed, 1 failed and 783 were skipped.

failed: Clang.CodeGen/stack-clash-protection.c

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Make some tests more portable.

Unit tests: pass. 62269 tests passed, 0 failed and 827 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 16 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision is now accepted and ready to land.Feb 6 2020, 10:02 AM

Any plans to merge this feature to 10 release?

Any plans to merge this feature to 10 release?

I would love to have it in -10 for Firefox !

I would love too, @hans what do you think?

This revision was automatically updated to reflect the committed changes.

This breaks check-clang on mac and win: http://45.33.8.238/mac/7485/step_7.txt http://45.33.8.238/win/7753/step_7.txt

Please revert and then investigate asynchronously, unless the fix is obvious.

clang/test/CodeGen/stack-clash-protection.c
3

Tests should compile binaries and then run them if at all possible. This is impossible in cross-build scenarios and in general we try hard to have unit tests, not integration tests. Check that this produces the IR you expect, and have an llvm-level test that the IR produces the assembly you expect, and if you must, an lld level test that checks that the generated elf file looks like you think.

Reverted in b03c3d8c620 for now.

Also got failed clang tests on Arm/Aarch64 builders

http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/4295
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/4277

  • FAIL: Clang::stack-clash-protection.c
  • FAIL: Clang::stack-clash-protection.c
error: unable to create target: 'No available targets are compatible with triple "x86_64"'

looks like REQUIRES: x86-registered-target keyword is needed.

For the record: tests have been updated, option handling cleaned up and expansive check failure fixed in commit e67cbac81211d40332a79d98c9d5953624cc1202.

sberg added a subscriber: sberg.Aug 10 2020, 12:13 PM
sberg added inline comments.
clang/include/clang/Driver/Options.td
1770

Should this rather spell "fno-stack-clash-protection"? The above change to clang/docs/ClangCommandLineReference.rst (but which got overwritten by https://github.com/llvm/llvm-project/commit/9624beb38a46037f69362650a52e06d8be4fd006 "[docs] Regenerate ClangCommandLineReference.rst") mentions -fno-stack-clash-protection, and also GCC calls it like that. (Though the below clang/test/Driver/stack-clash-protection.c does use -fnostack-clash-protection.)

xbolva00 added inline comments.Aug 10 2020, 12:18 PM
clang/include/clang/Driver/Options.td
1770

And the fix should be backported to release branch.

@hans

sberg added a comment.Aug 12 2020, 9:10 AM

I filed https://bugs.llvm.org/show_bug.cgi?id=47139 "Misspelled -fnostack-clash-protection" now.

kees reopened this revision.Aug 27 2020, 4:31 PM
kees added a subscriber: kees.

Sorry if I missed something here, but why is this marked as "Closed"? It seems like the feature has still not landed (i.e. it got reverted).

This revision is now accepted and ready to land.Aug 27 2020, 4:32 PM
efriedma closed this revision.Aug 27 2020, 4:38 PM

It was reverted, then re-landed. It's in trunk now.

kees added a comment.Aug 28 2020, 12:23 AM

Ah! Yes, I see it now. Thanks and sorry for the noise!

emaste added a subscriber: emaste.Tue, Nov 24, 5:34 PM
emaste added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
2996–2997

Is there anything OS-dependent here?

I plan to add EffectiveTriple.isOSFreeBSD() to FreeBSD-s in-tree Clang
https://reviews.freebsd.org/D27366
and intend to upstream the change (with a test), but should we just remove the OS test completely?

clang/lib/Driver/ToolChains/Clang.cpp
2996–2997

According to https://blog.qualys.com/vulnerabilities-research/2017/06/19/the-stack-clash, stack clash impacts

Linux, OpenBSD, NetBSD, FreeBSD and Solaris

We should probably enlarge the test to these platforms.