Compile libraries with cf-protection=branch on x64

[erikcorry]

The CET control-flow enforcement technology feature on x64 prevents control-flow hijacking via corrupted vtables and similar. It requires a special NOP at all entry points that are targeted by indirect branches/calls.

For an executable to be protected, all object files linked into it have to have support. Until now, any rust object file would disable support for the entire executable. This adds the flag (if available) so that rust system libraries do not poison the protection when linked in.

Note: This doesn't do anything for shadow stack support. The two features can be enabled separately, so it's valuable in itself.

tracking issue: #93754

[rustbot]

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: bootstrap
• bootstrap expanded to 6 candidates
• Random selection from Mark-Simulacrum, clubby789, jieyouxu

[erikcorry]

Perhaps @abrown is a better reviewer for this?

[jieyouxu]

Is there more context to this? This would build x86_64 dist artifacts (including stable artifacts) with an unstable branch protection flag? That seems... strange?

View changes since this review

[erikcorry]

V8 and LLVM have support for -fcf-protection=branch, which is a great security feature, and actually very simple. It merely bans indirect jumps to locations that are not marked with the ENDBR64 instruction (which is just a NOP if the feature is not enabled on your CPU). This prevents a whole class of program-counter hijacking attacks (ROP).

However, currently you have to disable the parts of V8 written in rust for it to work, because the rust code pulls in artifacts that don't have the support (those NOP/ENDBR64 instructions at entry points). As more of V8 gets written in rust this problem is only going to get worse. Effectively, having part of your program written in rust means you can't get this performance-neutral defense-in-depth feature.

I also work on Cloudflare Workers, which embed V8 and have the same problem.

The LLVM support for cf-protection=branch is not considered experimental, but in rust the discussion (see linked bug) has been mostly about how to activate it. A flag is available in nightly, but currently doesn't work because of this issue.

[erikcorry]

I fixed it so that it only changes the artifacts on the non-stable build, but not sure how to do the same for the change in build.rs. This is for the optimized builtins which are in C, so it's a flag for the C compiler. I could perhaps do something tricky with environment, unless you have a better idea?

[tgross35]

Can't you build std yourself with the unstable flag for now?

If you have spare cycles to spend on improving the situation, it's probably better to work toward moving the relevant flags toward stabilization. That way once we have build-std, which is in progress, you'll be able to do what is needed from stable Rust.

[Jules-Bertholet]

IIUC, the flag is entirely useless without build-std, so there would be no point in stabilizing it alone.

[tgross35]

I think it's still useful to indicate that the design work has been done and it's a flag that can be relied on. But it could also be made effectively stabilization-ready even if stabilization itself is blocked on build-std, 🤷‍♂️.

[jieyouxu]

I wouldn't be against stabilizing that flag (given the usual well-testedness and no-blocking-bugs) even if its effectively useless without build-std, as long as we are clear about that limitation on stabilization with intent to change the prebuilt dist artifacts to flip the default to w/ protection in the future. That would be a step that I would like to see so we have some stronger confidence in this flag's impl and behavior.

Basically, IMO we should not and cannot be building and distributing artifacts built with this flag while its unstable.

Feel free to discuss this further in https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler.

@rustbot blocked (should not be building stable dist artifacts while this flag is still unstable)

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[erikcorry]

FWIW rustc is already an artifact that is compiled with unstable flags that gets shipped.

To be clear, the current version of this change doesn't affect artifacts that are shipped in releases, only nightly. I guess the purpose of nightly is to gather experience with flags that were not yet stabilized? Without this change it's not really possible to get experience with the flag, since the libs poison any executable you link and disable the feature.

[jieyouxu]

I'll discuss this with other compiler folks EDIT: this is now mostly a change to the prebuilt std

[jieyouxu]

FWIW rustc is already an artifact that is compiled with unstable flags that gets shipped.

Yes -- there are some unstable flags for various reasons, but e.g. for more "feature"-like ones we do not want to enable more unstable features lightly for stable artifacts.

To be clear, the current version of this change doesn't affect artifacts that are shipped in releases, only nightly.

Sorry I missed this, this makes sense to me. I'd at least be up for experimenting on nightly (just that we should not be building stable artifacts with the unstable flag; the current set of changes in this PR is for building nightly artifacts).

[jieyouxu]

@rustbot review

[jieyouxu]

Hm, this should probably get a libs reviewer since this primarily affects pre-build std. I'm at least on-board with experimenting on nightly-only (for now, before flag is stabilized).

r? libs

[wesleywiser]

FWIW, I'm not sure this is really a libs issue (if libs wants to weigh in though, by all means!).

The main questions I would be interested in are:

1. What is the binary size overhead for enabling this? As this is just an extra instruction at the start of each function, I would assume it's quite small.
2. Is there runtime overhead to endbr64?
3. Will we want to guarantee that the precompiled standard library crates will be built with this mitigation going forward?

[erikcorry]

Answer to question 1 from @wesleywiser :

Total sysroot libs: +74,574 bytes (+0.56%) (13.3 MB -> 13.4 MB)

More detailed look:

• Large libraries don't grow much: core +0.15%, std +0.19%, gimli +0.09%, test +0.21%
• Compiler-builtins grow quite a lot because they are many tiny functions, but usually LLVM is just generating an instruction for these operations, so they don't get (statically) linked in unless it's something obscure like 128bit div: +2.70% (+41 KB (note, this is the rust builtins, not the optimized C ones, which are not fixed yet in this PR).
• std.so shared library: +10,416 bytes (+0.70%).

[erikcorry]

Answer to question 2 from @wesleywiser:

I ran the coretests and the alloctests and it was basically in the noise, as you would expect with NOP-like instructions.

35% of the tests were faster, 34% within +/-1%, 30% slower.

Overall performance was -0.11%

My machine is a recent Xeon that has support for the instruction. It's not decoding it like a NOP, but like an ENDBR64 instruction. However, the benchmarking executable did not have the bit set, so the kernel didn't set the CPU to the mode where the rules would be enforced.

[erikcorry]

Will we want to guarantee that the precompiled standard library crates will be built with this mitigation going forward?

This isn't really something that is for me to have an opinion on. For now this allows people on the nightly builds to start enabling it so they can gain experience. Perhaps at some point it becomes standard or there is a way to use a different set of standard library artifacts when enabling it. I think that's a separate decision.