This is an archive of the discontinued LLVM Phabricator instance.

[X86] Control-Flow Enforcement Technology - Shadow Stack and Indirect Branch Tracking support (Clang side)
ClosedPublic

Authored by oren_ben_simhon on Nov 19 2017, 5:29 AM.

Details

Summary

Control Flow Enforcement Technology (CET) provides HW capabilities to defend against Return Oriented Programming (ROP) attack and similarly Call/Jmp Oriented Programming (COP/JOP) attack.
Control flow subversion attacks are handled using two CET’s mechanisms:

  1. Shadow Stack (SHSTK) – return address protection to defend against ROP.
  2. Indirect Branch Tracking (IBT) – free branch protection to defend against JOP/COP.

Shadow stack solution introduces a new stack for return addresses only. The stack has a Shadow Stack Pointer (SSP) that points to the last address to which we expect to return. If we return to a different address an exception is triggered.
This patch includes shadow stack intrinsics as well as the corresponding CET header. It includes CET clang flags for shadow stack and Indirect Branch Tracking.

For more information, please see the following:
https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane added inline comments.Nov 20 2017, 8:44 AM
include/clang/Driver/Options.td
1801 ↗(On Diff #123497)

Since CET is a union of the other two, how do we handle conflicting configurations? Is -mcet -mno-ibt an error, or the same as mno-shstk? Repeat question for all combinations.

craig.topper edited subscribers, added: cfe-commits; removed: llvm-commits.Nov 20 2017, 4:02 PM
craig.topper added inline comments.Nov 20 2017, 4:19 PM
include/clang/Basic/BuiltinsX86.def
642 ↗(On Diff #123497)

Space after commas to match the rest of the file

include/clang/Driver/Options.td
1801 ↗(On Diff #123497)

All the flags should go in the "X86 feature flags" section later in this file.

lib/Basic/Targets/X86.cpp
619 ↗(On Diff #123497)

It looks like "cet" is meant to be a alias for two features? Should you avoid putting it in Features map at line 545 lke we do with "sse4"? Then it will never get sent to the backend and you can remove FeatureCET from X86.td

620 ↗(On Diff #123497)

Is -mno-cet intended to disable both features? Cause that doesn't happen with the way this is written.

687 ↗(On Diff #123497)

This shouldn't be needed. "+cet" implies +shstk and +ibt. Those should be added to feature map before we call this method

1230 ↗(On Diff #123497)

I'm not sure what hasFeature is really used for, but I doubt "cet" needs to be in it.

test/CodeGen/builtins-x86.c
260 ↗(On Diff #123497)

I don't think you need to update this test. The intrinsic header test should be enough. I don't know why this test exists.

test/Driver/x86-target-features.c
75 ↗(On Diff #123497)

Should this check that +shstk and +ibt got added. That seems like the more important thing to check.

oren_ben_simhon marked 8 inline comments as done.Nov 21 2017, 12:09 PM
oren_ben_simhon added inline comments.
include/clang/Driver/Options.td
1801 ↗(On Diff #123497)

Since 'mcet' flag (which was supposed to superset the other two) over complicates things, I prefer to remove it at the moment.

test/CodeGen/builtins-x86.c
260 ↗(On Diff #123497)

I believe that it is for the case where someone is using the builtin directly instead of using the intrinsic (although this is not recommended). Builtin and intrinsic, although relate to each other, might not converge in the future. In that case it might be worth testing.

oren_ben_simhon marked an inline comment as done.

Implemented comments posted until 11/20 (Thanks Craig and Erich).

craig.topper added inline comments.Nov 21 2017, 8:53 PM
include/clang/Basic/BuiltinsX86_64.def
63 ↗(On Diff #123835)

Space after commas here too.

test/Preprocessor/x86_target_features.c
336 ↗(On Diff #123835)

Should this -mcet case be removed?

oren_ben_simhon marked 2 inline comments as done.

Implemented comments posted until 11/21 (Thanks Craig)

This revision is now accepted and ready to land.Nov 22 2017, 9:48 AM
This revision was automatically updated to reflect the committed changes.