Add support for control-flow protection #93439
Conversation
rustbot
added
the
T-compiler
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
abrown
force-pushed
the
cf-protection
branch
2 times, most recently
from
dfe5a84 to
20c4504
Compare
This comment has been minimized.
This comment has been minimized.
The statically linked bits in current Linux distributions are CET-enabled. With binutils |
You could search for |
petrochenkov
added
S-waiting-on-author
| *slot = match v { | ||
| None | Some("none") => CFProtection::None, | ||
| Some("branch") => CFProtection::Branch, | ||
| Some("return") => CFProtection::Return, | ||
| Some("full") => CFProtection::Full, | ||
| Some(_) => return false, | ||
| }; |
There was a problem hiding this comment.
It seems like it'd be pretty hard to extend this kind of CLI in a sensible manner in the future if the underlying CF protection concept changes in any way.
There was a problem hiding this comment.
To address this and your comments below about wanting a unified -Zbranch-protection: I agree that it would be nice to unify these various properties (see -Ccf-guard as well) but I think that could reasonably be discussed in a separate issue, mainly because I suspect the various flags are not exactly equivalent and are implemented with different assumptions (e.g., cf-protection expects certain ELF tags, kernel support, and architecture support). If such an issue exists, I would be glad to comment there. But I would not expect to have to figure that out in this PR. I was hoping this could just add support with the same flags that Clang exposes (-fcf-protection=none|branch|return|full).
|
as a side note, I think ideally we'd figure out an unified interface with |
@fweimer-rh: I started down this path but I first ran into the issue that I didn't even know how the linker is being called. Using the $ RUSTC_LOG=rustc_codegen_ssa::back::link=info rustc-custom -v -Z cf-protection=full -o empty-custom-rust empty.rs
INFO rustc_codegen_ssa::back::link preparing Executable to "empty-custom-rust"
INFO rustc_codegen_ssa::back::link "cc" "-m64" "empty-custom-rust.empty.ea9ce8e5-cgu.0.rcgu.o" "empty-custom-rust.empty.ea9ce8e5-cgu.1.rcgu.o" "empty-custom-rust.empty.ea9ce8e5-cgu.2.rcgu.o" "empty-custom-rust.empty.ea9ce8e5-cgu.3.rcgu.o" "empty-custom-rust.empty.ea9ce8e5-cgu.4.rcgu.o" "empty-custom-rust.empty.ea9ce8e5-cgu.5.rcgu.o" "empty-custom-rust.empty.ea9ce8e5-cgu.6.rcgu.o" "empty-custom-rust.30bkn99o4ddymxvy.rcgu.o" "-Wl,--as-needed" "-L" [... some rlib files]I could then try to run |
|
r? @nagisa |
@abrown Presumably |
|
@fweimer-rh, thanks, that worked! When I run Click to expand the rather large list of non-CET objectsThe summary of this is what I suspected: none of the dependencies are CET-enabled so LLVM must seek the the "lowest common denominator." These dependencies include libstd, libpanic_unwind, libobject, libgimli, liballoc, libcore, etc.--pretty much everything. Is there any way in the rustc toolchain to say something like "recompile all dependencies of this object with this flag"? Alternately, it has been suggested to me that this CET support might require a new target arch, e.g. |
|
There is |
@abrown Distributions have switched to building statically linked bits with CET enabled (and shared objects anyway), maybe the Rust distribution should do the same. It's relatively easy to do because all x86-64 implementations support long NOPs. It's more difficult for i586 because long NOPs are not part of the ISA there. |
|
@nagisa, @fweimer-rh: thanks for the help. Indeed the following command generates a CET-enabled binary: $ RUSTFLAGS="-Z cf-protection=full" RUSTC="rustc-custom" cargo +nightly build -Z build-std --target x86_64-unknown-linux-gnu
...
$ readelf -a target/x86_64-unknown-linux-gnu/debug/empty | grep feature:
Properties: x86 feature: IBT, SHSTKWhat else do you think is needed in this PR for it to be ready for review--docs, tests? [edit: @fweimer-rh, I think the idea of switching the default to CET-enabled could be discussed in a separate issue/PR?] |
Unstable features do benefit from docs, yeah. These generally go to the unstable book. Source for it is in You may also want to create an issue with the Exhaustive tests can be added as a follow-up, I feel, but here are some basic ones which I think are a must-have before this PR can land:
For the first two I recommend the branch-protection test as an example to follow.
Switching whatever defaults as a follow-up change would make landing this initial work and whatever experimentation much more straightforward for all parties involved. |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 8d6c973 with merge abd8bddece4c55023e7031274944620dd3799e56... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 8d6c973 with merge 5c0ef97094e15c18102978f62856cb6fdf059dc8... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry doesn't necessarily look related to the changes here unless we got some sort of a llvm pathology. |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (09cb29c): comparison url. Summary: This benchmark run did not return any relevant results. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
This change adds a flag for configuring control-flow protection in the LLVM backend. In Clang, this flag is exposed as
-fcf-protectionwith optionsnone|branch|return|full. This convention is followed forrustc, though as a codegen option:rustc -Z cf-protection=<none|branch|return|full>. Tracking issue for future work is #93754.