-
Notifications
You must be signed in to change notification settings - Fork 849
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
Enables Conditional Compiler Protections #6743
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, will check more tomorrow.
CMakeLists.txt
Outdated
# if we are building on a GLIBC environment, FORTIFY is available | ||
check_c_compiler_flag(-D_FORTIFY_SOURCE CC_SUPPORTS_FORTIFY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this ever fail? Since you're just setting the _FORTIFY_SOURCE
preprocessor symbol, it should not be possible for this to fail since the flag is always accepted for C compilers.
You probably need to construct a small program that should generate warning, compile it, and set this flag if the program emits a warning. Something like this:
# if we are building on a GLIBC environment, FORTIFY is available | |
check_c_compiler_flag(-D_FORTIFY_SOURCE CC_SUPPORTS_FORTIFY) | |
include(CheckCSourceCompiles) | |
set(CMAKE_REQUIRED_FLAGS -Werror) | |
set(CMAKE_REQUIRED_DEFINITIONS -D_FORTIFY_SOURCE) | |
# If this build succeeds, _FORTIFY_SOURCE is not available | |
check_c_source_compiles(" | |
#include <string.h> | |
int main() { | |
char buffer[8]; | |
strcpy(buffer, \"hello world\"); | |
} | |
" FORTIFY_NOT_AVAILABLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final comments.
CMakeLists.txt
Outdated
message(STATUS "Compiler ${CMAKE_C_COMPILER_ID} does not support -D_FORTIFY_SOURCE=1") | ||
endif() | ||
# CFI is currently supported by clang | ||
check_c_compiler_flag(-CFI CC_SUPPORTS_CFI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using CLang it does not have any -CFI
option. I see the following options though:
mats@fury:~$ clang --help | grep cfi
-fno-sanitize-cfi-canonical-jump-tables
-fno-sanitize-cfi-cross-dso
-fsanitize-cfi-canonical-jump-tables
-fsanitize-cfi-cross-dso
-fsanitize-cfi-icall-generalize-pointers
-mlvi-cfi Enable only control-flow mitigations for Load Value Injection (LVI)
-mno-lvi-cfi Disable control-flow mitigations for Load Value Injection (LVI)
CMakeLists.txt
Outdated
# CFI is currently supported by clang | ||
check_c_compiler_flag(-CFI CC_SUPPORTS_CFI) | ||
if(CC_SUPPORTS_CFI AND CC_SUPPORTS_LTO) | ||
add_compile_options(-flto -fsanitize=cfi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need the flag -fvisibility=hidden
here as well for this to compile. I get an error when trying:
mats@fury:~/lang/cc/examples$ clang -flto -fsanitize=cfi struct.c
clang: error: invalid argument '-fsanitize=cfi' only allowed with '-fvisibility='
mats@fury:~/lang/cc/examples$ clang -flto -fvisibility=hidden -fsanitize=cfi struct.c
mats@fury:~/lang/cc/examples$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkindahl which platform you compiled on? Fedora 39/gcc/x86_64 was not able to reproduce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using Ubuntu 22.04 LTS.
mats@fury:~/lang/cc/examples$ lsb_release --all
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 22.04.4 LTS
Release: 22.04
Codename: jammy
mats@fury:~/lang/cc/examples$ clang --version
Ubuntu clang version 14.0.0-1ubuntu1.1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
@@ -274,8 +274,28 @@ if(CMAKE_C_COMPILER_ID MATCHES "GNU|AppleClang|Clang") | |||
"The compiler ${CMAKE_C_COMPILER_ID} does not support -fvisibility=hidden" | |||
) | |||
endif(NOT CC_SUPPORTS_VISIBILITY_HIDDEN) | |||
# security options go here - these are not supported by all compilers | |||
# first and foremost, do we have LTO? | |||
check_c_compiler_flag(-flto CC_SUPPORTS_LTO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can be pretty sure that if the -fsanitize=cfi
works, -flto
will work as well, so no need to check this separately unless you want to provide a very specific error message saying what is wrong (and you do not have such a message here).
To enable Clang’s available CFI schemes, use the flag -fsanitize=cfi. You can also enable a subset of available schemes. As currently implemented, all schemes rely on link-time optimization (LTO); so it is required to specify -flto, and the linker used must support LTO, for example via the gold plugin.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6743 +/- ##
==========================================
+ Coverage 80.06% 80.97% +0.90%
==========================================
Files 190 191 +1
Lines 37181 36422 -759
Branches 9450 9463 +13
==========================================
- Hits 29770 29493 -277
- Misses 2997 3157 +160
+ Partials 4414 3772 -642 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Check CMake Files" check fails for this PR. You probably want to run make format
in the build directory to format all CMake files.
The following PR addresses the matter of Compiler Binary Protections. Specifically: