This is an archive of the discontinued LLVM Phabricator instance.

[x86][seses] Add clang flag; Use lvi-cfi with seses
ClosedPublic

Authored by zbrid on May 13 2020, 2:36 PM.

Details

Summary

This patch creates a clang flag to enable SESES. This flag also ensures that
lvi-cfi is on when using seses via clang.

SESES should use lvi-cfi to mitigate returns and indirect branches.

Diff Detail

Event Timeline

zbrid created this revision.May 13 2020, 2:36 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 13 2020, 2:36 PM
zbrid retitled this revision from [WIP][seses] Add clang flag; Use lvi-cfi with seses to [x86][seses] Add clang flag; Use lvi-cfi with seses.May 13 2020, 2:50 PM
craig.topper added inline comments.May 13 2020, 5:05 PM
clang/docs/ClangCommandLineReference.rst
2632

This file is in alphabetical order and is normally generated by a running clang-tblgen. See comment at the top of the file.

Took a quick look and seems sane -- will look after Craig's comment is addressed and build is passing

sconstab added inline comments.May 26 2020, 4:28 PM
clang/lib/Driver/ToolChains/Arch/X86.cpp
200

Would it be better to add FeatureLVIControlFlowIntegrity as a dependency for FeatureSpeculativeExecutionSideEffectSuppression in llvm/lib/Target/X86/X86.td?

llvm/lib/Target/X86/X86SpeculativeExecutionSideEffectSuppression.cpp
90

Is it really necessary to have the target feature and the CLI flag? If SESES is required for, say, a *.ll file, then +seses can always be added as a target feature.

Any progress on this patch? D75939 has been merged, but the SESES feature will not be secure until it has CFI protections.

zbrid marked 2 inline comments as done.Jun 17 2020, 12:34 PM

Thanks for the ping, Scott. I'll update this so I can get it submitted soon.

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

Thanks for the tip! Yeah, I will update to do that, but it looks like that only ensures an error will be thrown if they aren't used together rather than ensuring one is enabled when the other is enabled. Am I misunderstanding?

llvm/lib/Target/X86/X86SpeculativeExecutionSideEffectSuppression.cpp
90

I think there should be a way to turn on SESES without lvi-cfi. Similar to how there are flags to turn on SLH in various configurations. I'll see if I can lower the number of flags while still enabling that possibility.

zbrid updated this revision to Diff 272105.Jun 19 2020, 9:33 AM
zbrid marked an inline comment as done.

Update Clang Command Ref with automated tool

llvm/lib/Target/X86/X86SpeculativeExecutionSideEffectSuppression.cpp
90

Ah I think I'll change the SESES-only flag to enable-without-lvi-cfi, so it's more explicit it's missing functionality/security. Updates will come soon.

zbrid marked 2 inline comments as done.Jun 19 2020, 9:33 AM
zbrid updated this revision to Diff 272114.Jun 19 2020, 9:50 AM

seses implies lvi-cfi

also enable-seses -> enable-seses-without-lvi-cfi

zbrid added a comment.Jun 19 2020, 9:58 AM

@sconstab @craig.topper @mattdr -- This is ready for another round of review.

zbrid updated this revision to Diff 272117.Jun 19 2020, 10:00 AM

Fix accidentally deleted clang command line ref

MaskRay added inline comments.
clang/include/clang/Driver/Options.td
2345

CoreOption is accepted by clang-cl. You need a %clang_cl test if you use CoreOption.

Harbormaster completed remote builds in B61055: Diff 272117.
zbrid marked 2 inline comments as done.Jun 19 2020, 11:10 AM
zbrid added inline comments.
clang/include/clang/Driver/Options.td
2345

Is there a typical place to put this test? Is this a .cc -> LLVM IR test that's wanted? Any examples you can point to?

sconstab accepted this revision.Jun 29 2020, 1:14 PM

LGTM.

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

I'm not certain about this either. @craig.topper opinion?

This revision is now accepted and ready to land.Jun 29 2020, 1:14 PM
craig.topper added inline comments.Jun 29 2020, 1:58 PM
clang/lib/Driver/ToolChains/Arch/X86.cpp
200

Making them dependent in X86.td will make +sese imply +lvi-cfi and make -lvi-cfi imply -sese. So sese can never be enabled without lvi-cfi also enabled.

zbrid marked 3 inline comments as done.Jul 7 2020, 11:16 AM
zbrid updated this revision to Diff 276153.Jul 7 2020, 11:28 AM

update seses flag

zbrid updated this revision to Diff 276196.Jul 7 2020, 1:19 PM

rebase prior to commit

This revision was automatically updated to reflect the committed changes.