Page MenuHomePhabricator

[X86] Support customizing stack protector guard
ClosedPublic

Authored by xiangzhangllvm on Sep 30 2020, 7:57 PM.

Details

Summary

Some users want more freedom to customize the stack protector guard:

Sync with GCC
Let Clang Support

-mstack-protector-guard=tls/global,
-mstack-protector-guard-reg=fs/gs,
-mstack-protector-guard-offset=(num)

Let LLVM Support

-stack-protector-guard=tls/global,
-stack-protector-guard-reg=fs/gs,
-stack-protector-guard-offset=(num)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2020, 7:57 PM
xiangzhangllvm requested review of this revision.Sep 30 2020, 7:57 PM
xiangzhangllvm added a reviewer: wxiao3.
craig.topper added inline comments.Sep 30 2020, 8:22 PM
clang/lib/Driver/ToolChains/Clang.cpp
3040

Probably shouldn't push the arg if its not valid.

3048

Verify that this parsed correctly?

3056

The enum also has SS in it. Should we allow it here?

llvm/include/llvm/Target/TargetOptions.h
83

These register names are X86 specific. Should we be storing a string and let the relevant backend deal with it?

255

"fs or gs" should be global or tls right?

craig.topper added inline comments.Sep 30 2020, 8:28 PM
clang/include/clang/Basic/CodeGenOptions.def
360

If this defaults to 40 won't that get passed to the backend and override the -1 check in X86TargetLowering::getIRStackGuard that is picks different values for 32 and 64 bit?

MaskRay added inline comments.Sep 30 2020, 10:42 PM
clang/include/clang/Driver/Options.td
2251

You can drop DriverOption (clang::driver::options::DriverOption bit). It is mostly useless now. Ditto below

2252

No period in the help message

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

A->render(Args, CmdArgs)

clang/test/Driver/stack-protector-guard.c
2

x86_64 if it works for other ELF x86_64 OSes.

8

For not %clang tests, it is best to add a -c to avoid the linker stage.

16

The prevailing form is to place | \ at the end and indent the continuation line by 2.

llvm/include/llvm/CodeGen/CommandFlags.h
102

These related declarations do not need empty lines

llvm/include/llvm/Target/TargetOptions.h
77

None as the first may be what most users would expect.

llvm/lib/CodeGen/CommandFlags.cpp
484
492
505
MaskRay added a comment.EditedSep 30 2020, 10:44 PM

Sync with GCC

You can reference https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81708

Let LLVM Support -stack-protector-guard=tls/global,

You can say llc instead of LLVM. For example, opt does not support llvm/lib/CodeGen/CommandFlags.cpp options.

xiangzhangllvm marked 16 inline comments as done.Oct 1 2020, 1:51 AM

@Craig and MaskRay , thank you very much for your careful reviews !!

clang/include/clang/Basic/CodeGenOptions.def
360

Yes, here if we don't set -mstack-protector-guard-offset=xxx, X86TargetLowering::getIRStackGuard will use value according to 32/64bits

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

I find gcc don't use ss, let's sync.

llvm/include/llvm/Target/TargetOptions.h
83

I find all the options use unsigned or enum type.
Can we rename it to {None, Reg0, Reg1} here?
if other target want support it, it can understand by itself.

Thanks for the patch! This is useful feature that allows for better protections; rather than having a single stack canary, it's now possible to have dynamic canary values. The Linux kernel uses this, though only for 2 non-x86 architectures at the moment; Aarch64 and PowerPC. Looking at its usage, here's my feedback for this patch:

  1. the -mstack-protector-guard-reg= needs to consider the target. As such if I use a different target, I shouldn't be able to specify x86 registers. There should be an error emitted, and test coverage added in this patch for that. (clang/test/Driver/stack-protector-guard.c has one test for unsupported reg. I would like to see a test for an x86 reg but Aarch64 target for example. That would ensure our frontend validation isn't blindly accepting x86 specific reg values).
  2. GCC also supports sysreg option for -mstack-protector-guard=, and it is used for Aarch64. It would be good to add support for that option here in the parsing, even if there's no sane values for x86 for the corresponding -mstack-protector-guard-reg=.
  3. There should be tests added and an error message for using these flags with unsupported architectures. I don't expect you to implement support for the various targets, but I would appreciate testing that they fail with a reasonable error message saying these options aren't implemented for the current target. I'd say this differs subtly from 1. in that 1. is more about regs for the wrong target, while 3. is more generally about the target being unsupported.
clang/include/clang/Driver/Options.td
2610

It looks like -mstack-protector-guard=sysreg is also supported by GCC.

clang/lib/CodeGen/BackendUtil.cpp
528

need a sysreg option, too.

534–535

These are going to be target dependent, right?

clang/lib/Driver/ToolChains/Clang.cpp
3035–3036

If this value of A is not reused post if statement, can these be combined and still fit on one line?

if (Arg *A = ...)

Also the * should be against the A, no space.

3059

Arch specific?

3060

Can we print something more helpful, like the list of supported values based on the target?

llvm/include/llvm/Target/TargetOptions.h
83

I agree with @craig.topper here.

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

remove unnecessary extra parens

Also, when introducing support for a new command line flag, the relevant documentation should be updated. It can also be helpful to note support was added in the release notes.

xiangzhangllvm marked 2 inline comments as done.
xiangzhangllvm marked 9 inline comments as done.Oct 7 2020, 6:09 PM

@nickdesaulniers thank you very much for your guide!
Sorry for the later update, I take a travel these days.

clang/include/clang/Driver/Options.td
2610

Done in clang.cpp

xiangzhangllvm marked an inline comment as done.Oct 15 2020, 12:12 AM

ping

I would like to see a test added for a non x86 architecture, from the previous round of comments:

There should be tests added and an error message for using these flags with unsupported architectures. I don't expect you to implement support for the various targets, but I would appreciate testing that they fail with a reasonable error message saying these options aren't implemented for the current target. I'd say this differs subtly from 1. in that 1. is more about regs for the wrong target, while 3. is more generally about the target being unsupported.

clang/include/clang/Basic/CodeGenOptions.h
330–332

Might be nice to have comments for these members?

clang/lib/CodeGen/BackendUtil.cpp
534–535

I don't think this was addressed.

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

Please sink this definition closer to its use below.

llvm/lib/CodeGen/CommandFlags.cpp
482–485

This is still target dependent. You need to check the target before checking target specific registers.

500

Would a StringSwitch be more appropriate here?

llvm/lib/CodeGen/StackProtector.cpp
384–387

Do we want to retain Guard's previous scope?

xiangzhangllvm edited the summary of this revision. (Show Details)
xiangzhangllvm marked 4 inline comments as done.Oct 15 2020, 11:45 PM

TKS for reviewing!

clang/lib/CodeGen/BackendUtil.cpp
534–535

Use string, let target deal with it now.

clang/test/Driver/stack-protector-guard.c
19

Add non-x86 arch here before here and this time add following 2 checks.

llvm/lib/CodeGen/CommandFlags.cpp
482–485

Done by directly passing string for GuardReg option.

500

It not convenient to hand to "none", and can I keep the same style with other functions in this file? because I didn't saw StringSwitch use in this file.

llvm/lib/CodeGen/StackProtector.cpp
384–387

I know your idea, I tried to change with following patch, but it got built fail.

-  Value *Guard = TLI->getIRStackGuard(B);
   auto GuardMode = TLI->getTargetMachine().Options.StackProtectorGuard;
-
-  if ((GuardMode == llvm::StackProtectorGuards::TLS ||
-       GuardMode == llvm::StackProtectorGuards::None) && Guard)
+  if ((Value *Guard = TLI->getIRStackGuard(B)) &&
+      (GuardMode == llvm::StackProtectorGuards::TLS ||
+       GuardMode == llvm::StackProtectorGuards::None))
LuoYuanke added inline comments.Oct 16 2020, 4:51 AM
llvm/lib/CodeGen/StackColoring.cpp
694 ↗(On Diff #298550)

It seems you messed two patches?

xiangzhangllvm marked 2 inline comments as done.Oct 18 2020, 7:46 PM
xiangzhangllvm added inline comments.
llvm/lib/CodeGen/StackColoring.cpp
694 ↗(On Diff #298550)

Oh! yes! I am so sorry!

xiangzhangllvm marked an inline comment as done.
MaskRay added inline comments.Oct 18 2020, 10:34 PM
clang/include/clang/Basic/CodeGenOptions.h
335

Suggest: The TLS base register when StackProtectorGuard is "tls". On x86 this can be "fs" or "gs".

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

We use TODO instead of TBD.

Suggest: TODO: Support "sysreg" for AArch64.

If AArch64 is not supported yet, please don't accept an AArch64 specific value.

xiangzhangllvm added inline comments.Oct 19 2020, 1:36 AM
clang/lib/Driver/ToolChains/Clang.cpp
3035

I tend to fix it, thanks your review!

And please let me sync with nick, because he has some suggestions here before.

@nickdesaulniers
I find gcc 10.2 didn't support sysreg, So if you don't mind, I tend to remove it in next update.

$gcc -O2 -fstack-protector-all  -mstack-protector-guard=sysreg t.c -S -o -
gcc: error: unrecognized argument in option ‘-mstack-protector-guard=sysreg’
gcc: note: valid arguments to ‘-mstack-protector-guard=’ are: global tls
clang/lib/Driver/ToolChains/Clang.cpp
3035

That's because:

  1. it's currently only implemented for aarch64-linux-gnu-gcc, try with that and you'll see.
  2. GCC is architecturally different than clang in that GCC needs to be configured to target one backend, always, while Clang defaults to all backends enabled but can be reconfigured to disable backends.

Maybe worthwhile to warn then that -mstack-protector-guard=sysreg is not supported by "this target" when this target is x86.

MaskRay added inline comments.Oct 19 2020, 2:07 PM
clang/lib/Driver/ToolChains/Clang.cpp
3035

The driver should match what has actually been implemented. Since only x86 supports the toggle, just don't accept AArch64 specific values.

For this case, just don't mention AArch64 or sysreg in the code. You can freely add a TODO.

xiangzhangllvm added inline comments.Oct 19 2020, 5:17 PM
clang/lib/Driver/ToolChains/Clang.cpp
3035

In my eye, I think MaskRay make sense. @nickdesaulniers, I'll mark TODO here for "sysreg", it is better to add it when AArch64 want fully implemented it.

xiangzhangllvm marked 3 inline comments as done.
nickdesaulniers accepted this revision.Oct 21 2020, 10:53 AM

LGTM; thanks!

clang/lib/CodeGen/BackendUtil.cpp
525–526

did clang-format wrap CodeGenOpts.StackProtectorGuard like this?

This revision is now accepted and ready to land.Oct 21 2020, 10:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2020, 7:08 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

TKS all review!!

Looks like there's also a -mstack-protector-guard-symbol= which Linux kernel developers are looking to use for 32b X86 kernels: https://bugs.llvm.org/show_bug.cgi?id=49209.