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

-C target_cpu=cortex-a72 (and -target-cpu=native on Raspberry Pi) wrongly enables crypto features that are optional on Cortex-A72 #125033

Open
briansmith opened this issue May 12, 2024 · 3 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. A-targets Area: Concerning the implications of different compiler targets C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-AArch64 Armv8-A or later processors in AArch64 mode P-critical Critical priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@briansmith
Copy link
Contributor

Building with RUSTFLAGS="-C target_cpu=cortex-a72" statically enables the target_feature="aes", target_feature="crc", target_feature="pmuv3", and target_feature="sha2". However, at least the crypto features AES, CRC, and SHA2 are optional on this CPU. The definition for this target is wrong. See the upstream LLVM bug: llvm/llvm-project#90365.

The main consequence is that crypto libraries that use cfg(target_feature = ...) feature detection for these hardware instructions are getting miscompiled, causing the programs to, at best, crash with an illegal instruction exception. This particular affects Raspberry Pi users compiling with RUSTLFAGS=-target-cpu=native. From briansmith/ring#1858 (comment):

$ rustc --print cfg --target=aarch64-unknown-linux-gnu -C target_cpu=cortex-a72 | grep feature
target_feature="aes"
target_feature="crc"
target_feature="neon"
target_feature="pmuv3"
target_feature="sha2"

Without -C target_cpu=cortex-a72 we get the correct feature flags:

$ rustc --print cfg --target=aarch64-unknown-linux-gnu | grep feature
target_feature="neon"

I verified this is an issue on Rust 1.61 stable, 1.78 stable, and rustc 1.80.0-nightly (6e1d947 2024-05-10).

Although some crypto libraries may work around this issue, these workarounds have negative consequences. In the case of ring's workaround, the result of the workaround is bloat and worse performance on all AArch64 CPUs that actually are guaranteed to have the crypto extensions (except on Fuchsia, Windows, and macOS).

@briansmith briansmith added the C-bug Category: This is a bug. label May 12, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 12, 2024
@Nilstrieb Nilstrieb added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-AArch64 Armv8-A or later processors in AArch64 mode A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. A-targets Area: Concerning the implications of different compiler targets and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 12, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 12, 2024
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

Since in our support list this arch has the most support:

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 13, 2024
@briansmith
Copy link
Contributor Author

briansmith commented May 17, 2024

From today's comment in the upstream LLVM issue:

For -mcpu=xyz, we enable the maximal set of features for the cpu (so long as they are relatively common), which can be disabled with +nofeat. [....] The idea is that users get decent performance by default, and if they have less features can turn down the default.

Unfortunately GCC didn't follow that scheme at the time for crypto instructions, and had them disabled by default. Clang did, so there was a difference in whether crypto was enabled. We did not decide to retro-actively change old CPU definitions (it could be a breaking change), but going forward "Armv-9" cpus have been changed to not include crypto by default.

Assuming that is accurate, there are a few interesting things:

  • Potentially the issue is much broader than Cortex-A* CPUs, as it seems like LLVM doesn't actually have a policy of only-required-features-by-default. I.e. the current behavior seems to be by design.
  • Assuming we want -C target_cpu to be safe by default, rustc cannot delegate its defaults to LLVM. rustc should turn off feature flags for optional features by default. Basically -C target_cpu is generally not a memory-safe option. This should be documented retroactively for old versions and fixed for newer versions.
  • Assuming cc-rs also wants to be "safe by default," it also needs to manually turn off feature flags for optional features by default when the compiler is clang, by passing additional flags.
  • This all would be a potentially-compatibility-breaking change.

@briansmith
Copy link
Contributor Author

Also, from the LLVM issue:

For -march=armv8.x-a we enable the minimal set of features (so all required extensions). Optional features can be added with +feat.

So, one workaround would be to use that. But, I don't see rustc providing a mechanism to choose the ARM architecture level instead of a specific CPU.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. A-targets Area: Concerning the implications of different compiler targets C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-AArch64 Armv8-A or later processors in AArch64 mode P-critical Critical priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants