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

cmake: Fix hardening flags #211

Merged
merged 1 commit into from
Jun 3, 2024
Merged

cmake: Fix hardening flags #211

merged 1 commit into from
Jun 3, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented May 23, 2024

During testing #93, a few issues were noticed:

  • -mbranch-protection=bti shouldn't be in C flags

The other change simplifies the summary code in #93.

@hebasto hebasto marked this pull request as draft May 23, 2024 13:09
@theuni
Copy link

theuni commented May 24, 2024

A few things:

  1. Master currently adds -mbranch-protection=bti to ldflags. Agree that it shouldn't be, and this change looks correct, just pointing out that this isn't something introduced by CMake.
  2. The meaning of core_base_interface is becoming quite fuzzy to me. What happens when we switch to the upstream CMake build for secp?
  3. c-i error below:
CMake Error at CMakeLists.txt:499 (target_link_libraries):
  Error evaluating generator expression:

    $<COMPILE_LANGUAGE:CXX,OBJCXX>

  $<COMPILE_LANGUAGE:...> may only be used to specify include directories,
  compile definitions, compile options, and to evaluate components of the
  file(GENERATE) command.

Do not pass `-mbranch-protection=bti` during link stage.
@hebasto hebasto marked this pull request as ready for review May 24, 2024 17:47
Comment on lines +508 to +509
$<$<NOT:$<CONFIG:Debug>>:-U_FORTIFY_SOURCE>
$<$<NOT:$<CONFIG:Debug>>:-D_FORTIFY_SOURCE=3>
Copy link
Owner Author

Choose a reason for hiding this comment

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

Dealing with an individual options within generator expressions is much easier.

@hebasto
Copy link
Owner Author

hebasto commented May 24, 2024

@theuni

A few things:

  1. Master currently adds -mbranch-protection=bti to ldflags. Agree that it shouldn't be, and this change looks correct, just pointing out that this isn't something introduced by CMake.

I left only this change, which effectively is pulled from the current #93.

  1. The meaning of core_base_interface is becoming quite fuzzy to me. What happens when we switch to the upstream CMake build for secp?

I'll submit a demo branch shortly.

@hebasto hebasto added the bug Something isn't working label May 25, 2024
@hebasto
Copy link
Owner Author

hebasto commented May 25, 2024

I'll submit a demo branch shortly.

@theuni

I've updated #192, which is currently based on #93. I hope it will answer your question and resolves your concerns regarding the meaning of core_base_interface.

@m3dwards
Copy link

m3dwards commented Jun 3, 2024

ACK e2f2f23

Tested with and without PR and can now see the hardening flag not being passed during linking on M1 Mac.

@hebasto hebasto merged commit efc356b into cmake-staging Jun 3, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants