Add LLVM CFI support to the Rust compiler by rcvalle · Pull Request #89652 · rust-lang/rust

rcvalle · 2021-10-07T22:50:34Z

This PR adds LLVM Control Flow Integrity (CFI) support to the Rust compiler. It initially provides forward-edge control flow protection for Rust-compiled code only by aggregating function pointers in groups identified by their number of arguments.

Forward-edge control flow protection for C or C++ and Rust -compiled code "mixed binaries" (i.e., for when C or C++ and Rust -compiled code share the same virtual address space) will be provided in later work as part of this project by defining and using compatible type identifiers (see Type metadata in the design document in the tracking issue #89653).

LLVM CFI can be enabled with -Zsanitizer=cfi and requires LTO (i.e., -Clto).

Thank you, @eddyb and @pcc, for all the help!

rust-highfive · 2021-10-07T22:50:37Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2021-10-07T22:50:37Z

⚠️Warning⚠️

These commits modify compiler targets. (See the Target Tier Policy.)

rcvalle · 2021-10-07T22:50:47Z

r? @eddyb

rcvalle · 2021-10-08T00:03:38Z

FYI, I'll be looking into the CI build failures over the next days.

eddyb · 2021-10-08T08:55:04Z

r? @nagisa / @nikic (also, this feels like it could use an MCP? esp. for the user-facing interface)

nagisa · 2021-10-08T11:04:38Z

Huh, there seem to be more caveats with CFI compared to other sanitizers as far as I can tell at a glance. I would like to see this be landed behind a -Zunstable-options or somesuch initially to give us more leeway in adjusting the specifics of the implementation and interactions with other ecosystems.

eddyb · 2021-10-08T11:09:55Z

Oh I had mistakenly thought the sanitizers were under -Z, from my discussions with @rcvalle - if it's -C I definitely agree it needs a -Z opt-in, I suspect this won't be stabilizable for a while (or maybe we'd stabilize a version without FFI CFI, but that would require later FFI CFI support to be opt-in to avoid breakage).

nagisa · 2021-10-08T11:26:21Z

Ah, my bad, sanitizers are in general still -Z flags, I was recalling some discussion about making some sanitizers stable (#47174) and thought that the actual implementation of this idea has already happened.

It hasn't yet. But we still need to make sure that we don't accidentally stabilize CFI alongside all other sanitizers at the same time.

rcvalle · 2021-10-08T18:20:51Z

Sorry, the -C in the summary was a typo. Fixed.

bors · 2021-10-26T21:30:55Z

⌛ Testing commit 325cff7d964ca211f1e20256d9801ab932d01250 with merge 6502cfa028831805c3368093a0f1c520cb42ce37...

bors · 2021-10-26T22:19:10Z

💔 Test failed - checks-actions

This commit adds initial documentation for LLVM Control Flow Integrity (CFI) support to the Rust compiler (see rust-lang/rust#89652 and rust-lang/rust#89653).

This commit adds initial documentation for LLVM Control Flow Integrity (CFI) support to the Rust compiler (see rust-lang#89652 and rust-lang#89653).

nagisa · 2021-10-27T09:07:24Z

@bors r+

bors · 2021-10-27T09:07:25Z

📌 Commit c5708ca has been approved by nagisa

bors · 2021-10-27T09:19:45Z

⌛ Testing commit c5708ca with merge a8f6e61...

bors · 2021-10-27T12:27:42Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing a8f6e61 to master...

rust-timer · 2021-10-27T13:58:48Z

Finished benchmarking commit (a8f6e61): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

rust-highfive assigned estebank Oct 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 7, 2021

rust-highfive assigned eddyb and unassigned estebank Oct 7, 2021