Make lto and linker-plugin-lto work the same for compiler_builtins

[maurer]

Fixes: #142284

This still needs more testing to make sure it doesn't bring back #118609

[rustbot]

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
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

[rustbot]

Some changes occurred in tests/ui/sanitizer

cc @rcvalle

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

[maurer]

mmaurer@joyeuse:~/github/rust-lang/rust/sample-crate$ RUSTFLAGS="-Clinker-plugin-lto -Clinker=clang -Clink-arg=-fuse-ld=lld -Zsanitizer=cfi" cargo +stage1 run -Zbuild-std -Zbuild-std-features --release --target x86_64-unknown-linux-gnu
[SNIP]
   Compiling sample-crate v0.1.0 (/usr/local/google/home/mmaurer/github/rust-lang/rust/sample-crate)
    Finished `release` profile [optimized] target(s) in 1.81s
     Running `/usr/local/google/home/mmaurer/github/rust-lang/rust/target/x86_64-unknown-linux-gnu/release/sample-crate`
Hello, world!

mmaurer@joyeuse:~/github/rust-lang/rust/sample-crate$ RUSTFLAGS="-Clinker-plugin-lto -Clinker=clang -Clink-arg=-fuse-ld=lld -Zsanitizer=cfi" cargo +nightly run -Zbuild-std -Zbuild-std-features --release --target x86_64-unknown-linux-gnu
[SNIP]
rustc-LLVM ERROR: Do not know how to promote this operator!
error: could not compile `compiler_builtins` (lib)
[SNIP]

So it does seem to also work for fully linked programs with a custom-built std. The remaining thing to check is whether a side-injected implementation of methods in compiler_builtins results in duplicate definitions issues. As far as the previously reported bug:

mmaurer@joyeuse:~/github/sandbox-rp2040$ git diff
diff --git a/.cargo/config.toml b/.cargo/config.toml
index e6676fc..1acd15f 100644
--- a/.cargo/config.toml
+++ b/.cargo/config.toml
@@ -5,23 +5,10 @@
 #     it is configured via the Embed.toml in the root of this project
 # - elf2uf2-rs loads firmware over USB when the rp2040 is in boot mode
 runner = "probe-run --chip RP2040"
+linker = "ld.lld"
 # runner = "cargo embed"
 # runner = "elf2uf2-rs -d"
 
-rustflags = [
-  "-C", "linker=flip-link",
-  "-C", "link-arg=--nmagic",
-  "-C", "link-arg=-Tlink.x",
-  "-C", "link-arg=-Tdefmt.x",
-
-  # Code-size optimizations.
-  #   trap unreachable can save a lot of space, but requires nightly compiler.
-  #   uncomment the next line if you wish to enable it
-  # "-Z", "trap-unreachable=no",
-  "-C", "inline-threshold=5",
-  "-C", "no-vectorize-loops",
-]
-
 [build]
 target = "thumbv6m-none-eabi"
 
diff --git a/Cargo.toml b/Cargo.toml
index 9890df4..e18b9b3 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,4 +1,4 @@
 [workspace]
-#resolver = "2"
+resolver = "2"
 members = ["test-itertools", "test-pio-proc", "demo"]
 
diff --git a/demo/Cargo.toml b/demo/Cargo.toml
index 7930469..c9a1b07 100644
--- a/demo/Cargo.toml
+++ b/demo/Cargo.toml
@@ -14,7 +14,7 @@ panic-probe = { version = "0.3", features = ["print-defmt"] }
 
 
 # We're using a Pico by default on this template
-rp-pico = "0.5"
+rp-pico = "0.9"
 test-itertools = { path = "../test-itertools" }
 test-pio-proc = { path = "../test-pio-proc" }
 
mmaurer@joyeuse:~/github/sandbox-rp2040$ cargo +stage1 build -Zbuild-std --release
    Finished `release` profile [optimized] target(s) in 0.20s
mmaurer@joyeuse:~/github/sandbox-rp2040$ 

(I had to slightly adjust the deps to deal with modern cargo, but it seems to work fine.)

I have also tested this patch on the current Android platform build, which does a lot of different things with LTO being on and off.

Is there anything else folks think we should test? cc @tgross35 in case he has ideas of ways this might break stuff.

[tgross35]

I don't know enough about LTO to know the potential pitfalls here, but from the blame it looks like there may be some history with this #113923, #118609, #119885. Cc @dianqk

[dianqk]

r? @dianqk

[dianqk]

When using -Zsanitizer=cfi, should compiler_builtins and #![no_builtins] be excluded?

(I might reply slowly recently.)

[maurer]

tl;dr: From CFI's perspective, yes, compiler_builtins should be included.

In an ideal world, compiler_builtins would be included. To do otherwise could cause some otherwise legal Rust code to crash under sanitization, and would weaken the security of non-crashing Rust code using CFI.

At a high level, enabling CFI or KCFI for a crate does two things:

1. It annotates indirect callsites with information about the expected type of the target
2. It annotates functions with information about what type they are.

KCFI transmits this information through the program text itself, CFI transmits it through a global data structure constructed through LTO.

If we actually exclude compiler_builtins / #![no_builtins], any indirect calls (e.g. through function pointers, dyn Fn, or anything like that) targeting a function defined in that crate would fault and take down the program for both KCFI and CFI (because effect 1 would be in the other crates, but effect 2 would not be in compiler_builtins). It would fix the immediate issue, because the immediate issue is in doing codegen for 1 inside a non-LTO'd crate. We could apply #[no_sanitize] to the specific compiler_builtins crate, which would cause external KCFI code to work, but CFI code would still be missing the annotations because they are transmitted through LTO in that model.

Finally, suppressing CFI or KCFI in compiler_builtins means that there will be an unguarded indirect jump in any program that uses one of these dynamically selected implementations. That means that an attacker with an overwrite primitive can cause that indirect jump to go anywhere, which is what CFI is trying to prevent. The transition for a program image from "0 reachable unguarded indirect jumps" to "1 reachable unguarded indirect jump" decreases the effectiveness of CFI.

[maurer]

I don't know enough about LTO to know the potential pitfalls here, but from the blame it looks like there may be some history with this #113923, #118609, #119885. Cc @dianqk

Re #118609, I manually tested to make sure I didn't bring that back in #142323 (comment)

[dianqk]

Thanks for your explanation, but your code doesn't get compiler_builtins participate LTO. You can run this to see which crates participated LTO.

RUSTC_LOG=rustc_codegen_llvm::back::lto rustc +stage1 -Clto=fat main.rs

IIUC, Clang has not applied CFI to compiler-rt.

Getting X participated in LTO is very tricky, or at least no one has ever tried it before.

@rustbot author

[rustbot]

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

[maurer]

Thanks for your explanation, but your code doesn't get compiler_builtins participate LTO. You can run this to see which crates participated LTO.

RUSTC_LOG=rustc_codegen_llvm::back::lto rustc +stage1 -Clto=fat main.rs

I will double check the logs with the right command, but for CFI (or my change) to work, you need a stdlib builtin that configuration, which isn't normally implied by just adding -Clto=fat - it's not going to rebuild compiler_builtins to get a bitcode-bearing version. You'd need to do a cargo -Zbuild-std build or manually provide your sysroot libraries in order to check.

Probably the right command for a dummy cargo package looks like

# Rust-driven-LTO variant (believed working today without my patch)
RUSTFLAGS="-Clto -Zsanitizer=cfi -C codegen-units=1" cargo build -Zbuild-std
# Linker-driven-LTO variant (definitely broken without my patch, I believe it's working with my patch)
RUSTFLAGS="-Clinker-plugin-lto -Zsanitizer=cfi -Clinker=clang -Clink-arg=-fuse-ld=lld" cargo build -Zbuild-std

I'll get back to you with a concrete dump of the change in compilation if you think this may not be working.

Given that the error went away, the only potential failure mode I expect is that the CFI in compiler-builtins might be in a segregated alias-space from the rest of the binary. That would be good to know, because it would mean the issue isn't entirely fixed, but it would mean it would be harder to trigger, because the user code would need to use a compiler-builtins function as a pointer, or compiler-builtins would need to take a user function pointer (not currently present).