Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add -fcf-protection. #4437

Merged
merged 7 commits into from
Jul 21, 2023
Merged

Add -fcf-protection. #4437

merged 7 commits into from
Jul 21, 2023

Conversation

JohanEngelen
Copy link
Member

@JohanEngelen JohanEngelen commented Jul 11, 2023

Resolves issue #4435

I do not know how to cleanly add compiler-defined enum value (e.g. __CET__ == 1), so I opted for adding compiler-defined versions instead.

CHANGELOG.md Outdated Show resolved Hide resolved
driver/main.cpp Outdated
@@ -1003,6 +1003,20 @@ void registerPredefinedVersions() {
VersionCondition::addPredefinedGlobalIdent("LDC_ThreadSanitizer");
}

switch (opts::fCFProtection) {
case opts::CFProtectionType::Branch:
VersionCondition::addPredefinedGlobalIdent("__CET_1__");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you wanted to add a enum instead of a bunch of version there is __traits(getInfo, "string_key")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__traits(getTargetInfo, "key"), might be a good idea. clang's __CET__ can be used as bitmask, whereas checking for branch-protection with the current versions would probably be something ugly like

version (__CET_1__) version = BranchCFProtection;
version (__CET_3__) version = BranchCFProtection;
version (BranchCFProtection) …

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getTargetInfo, that was what I meant to say. Also it is not immediately obvious what __CET_n__ refer to. I agree that adding VersionCondition::addPredefinedGlobalIdent("LDC_BranchCFProtection"); is much better.
(as an aside, does GCC/GDC have something similar? if so we should try to have a common interface "D_BranchControlFlowProtection"? @ibuclaw).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as an aside, does GCC/GDC have something similar?

GDC supports CET but I don't think it sets any predefined version

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the help here :)
__traits(getTargetInfo, "__CET__") for similarity with C ? Or without the underscores?
No version needed then, static if (__traits(getTargetInfo, "key") > 0) would suffice. I'm thinking that __CET__ is very rarely needed; the only usecase I can think of is writing assembly (I see in cet.h the bitmasking usecase the Martin mentioned).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as an aside, does GCC/GDC have something similar?

GDC supports CET but I don't think it sets any predefined version

Correct. The druntime library is compiled with -fversion=CET because I've not ported fibers to work with both branch protection or shadow stack.

I gather otherwise the end user shouldn't really care about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the help here :)
__traits(getTargetInfo, "__CET__") for similarity with C ? Or without the underscores?
No version needed then, static if (__traits(getTargetInfo, "key") > 0) would suffice.

BTW, I went with -fversion because the code that really needs to be aware of it in druntime (asm Fibers) is a bunch of nested version conditions and declarations.

@JohanEngelen
Copy link
Member Author

It turns out that @naked functions (with assembly implementation, such as fiber_switchContext), actually do get endbr added at the start of the function when CET is enabled. So they are not entirely naked, and we don't need to modify those druntime functions :) So perhaps things just work already without further modification needed.

@ibara Can you try building LDC's runtime with this patch and test the whole LDC test suite on OpenBSD (with CET requirement)?

@JohanEngelen
Copy link
Member Author

Should we error when this flag is passed for non-X86 targets?

@ErnyTech
Copy link

Should we error when this flag is passed for non-X86 targets?

Yes, Clang currently shows error: option 'cf-protection=branch' cannot be specified on this target
Without an error the user might think that the option is valid and the protection applied

@ibara
Copy link
Contributor

ibara commented Jul 13, 2023

It turns out that @naked functions (with assembly implementation, such as fiber_switchContext), actually do get endbr added at the start of the function when CET is enabled. So they are not entirely naked, and we don't need to modify those druntime functions :) So perhaps things just work already without further modification needed.

@ibara Can you try building LDC's runtime with this patch and test the whole LDC test suite on OpenBSD (with CET requirement)?

Will test this weekend. Thanks!

@kinke
Copy link
Member

kinke commented Jul 15, 2023

[LLVM 9 needs a fix: /home/runner/work/ldc/ldc/driver/cl_options_instrumentation.cpp:127:58: error: ‘const class llvm::Triple’ has no member named ‘isX86’]

@JohanEngelen
Copy link
Member Author

all green now

@ibara
Copy link
Contributor

ibara commented Jul 17, 2023

Didn't forget about this. Will get you test data when I am home a little later today.

@JohanEngelen JohanEngelen merged commit e03b084 into ldc-developers:master Jul 21, 2023
@JohanEngelen JohanEngelen deleted the bti branch July 21, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants