Disable CFI for core and std CFI violations #115200

rcvalle · 2023-08-25T03:21:46Z

Work around #115199 by temporarily disabling CFI for core and std CFI violations to allow the user rebuild and use both core and std with CFI enabled using the Cargo build-std feature.

rustbot · 2023-08-25T03:21:55Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

ehuss · 2023-08-25T04:07:51Z

r? compiler

I don't know who to assign this to.

compiler-errors · 2023-08-25T04:10:28Z

library/core/src/ops/function.rs

+        // FIXME: Rust's "try catch" construct (i.e., std::panicking::r#try) use
+        // of FnOnce explicitly violates CFI.


Is there a way to not have to tag this specific impl with no_sanitize(cfi)? Like putting it on something in panicking.rs instead? It seems a bit indirect to have to tag this impl for something that lives so far away.

Yeah, if we have to tag FnOnce::call_once for this, it raises questions as to whether LLVM's CFI implementation can support Rust at all, because this is a lot of closures.

workingjubilee · 2023-08-25T04:11:10Z

library/std/src/sys/unix/thread_local_dtor.rs

+// FIXME: Transmuting __cxa_thread_atexit_impl and indirectly branching
+// to/calling it here is an explicit CFI violation.
+#[no_sanitize(cfi)]


What exactly is the CFI violation, here? If the concern is simply that *mut u8 is not *mut c_void, then it should work if it casts the *mut u8 and such to *mut c_void, no?

Though... why does CFI care about the difference between *mut c_void and *mut u8?

(yeah, especially since c_void is repr(u8))

I just adjusted the casts/transmutes in this indirect call site (see #115199 (comment)). Let me know if it looks good to you. I'll take a look at the other two.

(I'll also take a look at the CI errors.)

Okay, excellent. I'm still concerned about the need to change this in the general sense, but this looks much more understandable.

library/std/src/sys/unix/thread_local_dtor.rs

rcvalle · 2023-09-13T17:01:22Z

FYI, I'm currently working on these--I just need a little bit more time.

bors · 2023-09-14T02:57:58Z

☔ The latest upstream changes (presumably #114656) made this pull request unmergeable. Please resolve the merge conflicts.

maurer · 2023-09-15T19:33:36Z

If we're landing workarounds by disabling CFI in particular places, can we land #[no_sanitize(kcfi)] adjacent to any instances of #[no_sanitize(cfi)]?

rcvalle · 2023-09-15T19:47:47Z

I'm not sure if we'll fix everything at once or temporarily land part of this PR to allow the user rebuild and use both core and std with CFI enabled in the meanwhile yet, but if we decide to do the later, I can definitely add #[no_sanitize(kcfi)] to it too.

I'm currently working on (3) from #115199, which is actually a bug in the CFI implementation, and either #[no_sanitize(cfi)] or #[no_sanitize(kcfi)] shouldn't be necessary there.

Noratrieb · 2023-09-19T09:17:19Z

library/std/src/sys/unix/thread_local_dtor.rs

+    /// std links to by default may be a C or C++ implementation that was not
+    /// compiled using the Clang integer normalization option.
+    #[cfg(not(sanitizer_cfi_normalize_integers))]
+    #[cfi_encoding = "i"]


why the explicit encoding? i would expect c_int to be an integer already as its repr(transparent).

This is the encoding of the C int type, which from a CFI perspective is distinct from i32 or any other rust type. int and long can be the same size and in that case would be the same rust type, but CFI does distinguish between them unless -Zcfi-normalize-integers is used to only record the integer type size rather than it's name.

Works around rust-lang#115199 by temporarily disabling CFI for core and std CFI violations to allow the user rebuild and use both core and std with CFI enabled using the Cargo build-std feature.

Updates documentation for LLVM CFI support with recommended information since the user can now rebuild and use both core and std with CFI enabled using the Cargo build-std feature.

rcvalle · 2023-10-04T17:56:22Z

I'd like to propose merging this PR, that works around (1) and (2) from #115199 by disabling CFI in those two specific locations, along with #116404, that fixes (3) also from #115199 (which the work around was removed from this PR). This would allow people to start experimenting with CFI and rebuild and use both core and std with CFI enabled, while I work on fixing (1) and (2).

workingjubilee

With the FnOnce concern being handled separately, this seems more reasonable.

workingjubilee · 2023-08-26T00:51:57Z

src/doc/unstable-book/src/compiler-flags/sanitizer.md

-
-[rust-book-ch19-05]: https://doc.rust-lang.org/book/ch19-05-advanced-functions-and-closures.html
-[rust-book]: https://doc.rust-lang.org/book/title-page.html
+Fig. 13. Build and execution of FIgs. 10–11 with LLVM CFI enabled.


Suggested change

Fig. 13. Build and execution of FIgs. 10–11 with LLVM CFI enabled.

Fig. 13. Build and execution of Figs. 10–11 with LLVM CFI enabled.

library/std/src/sys/unix/thread_local_dtor.rs

workingjubilee · 2023-10-04T18:42:33Z

@bors r+

bors · 2023-10-04T18:42:35Z

📌 Commit abece4f has been approved by workingjubilee

It is now in the queue for this repository.

rcvalle · 2023-10-04T18:45:29Z

Thank you for your time on this, @workingjubilee! Much appreciated.

bors · 2023-10-04T18:54:52Z

⌛ Testing commit abece4f with merge eea2614...

bors · 2023-10-04T20:43:26Z

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing eea2614 to master...

rust-timer · 2023-10-04T22:55:55Z

Finished benchmarking commit (eea2614): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 622.858s -> 623.925s (0.17%)
Artifact size: 271.98 MiB -> 272.03 MiB (0.02%)

rustbot assigned ehuss Aug 25, 2023

rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2023

rcvalle added the PG-exploit-mitigations Project group: Exploit mitigations label Aug 25, 2023

rcvalle force-pushed the rust-cfi-fix-115199 branch from ad98511 to 68f1697 Compare August 25, 2023 03:27

rustbot assigned b-naber and unassigned ehuss Aug 25, 2023

compiler-errors reviewed Aug 25, 2023

View reviewed changes

workingjubilee reviewed Aug 25, 2023

View reviewed changes

rcvalle force-pushed the rust-cfi-fix-115199 branch from 68f1697 to 477a586 Compare August 25, 2023 21:30

This comment has been minimized.

Sign in to view

rcvalle force-pushed the rust-cfi-fix-115199 branch from 477a586 to 0b62eb4 Compare August 25, 2023 21:40

This comment has been minimized.

Sign in to view

workingjubilee reviewed Aug 25, 2023

View reviewed changes

library/std/src/sys/unix/thread_local_dtor.rs Outdated Show resolved Hide resolved

library/std/src/sys/unix/thread_local_dtor.rs Show resolved Hide resolved

workingjubilee reviewed Aug 25, 2023

View reviewed changes

library/std/src/sys/unix/thread_local_dtor.rs Outdated Show resolved Hide resolved

rcvalle force-pushed the rust-cfi-fix-115199 branch from 0b62eb4 to d4ed0a0 Compare August 25, 2023 23:34

workingjubilee reviewed Aug 25, 2023

View reviewed changes

library/std/src/sys/unix/thread_local_dtor.rs Outdated Show resolved Hide resolved

maurer mentioned this pull request Sep 12, 2023

rust: Enable KCFI support Rust-for-Linux/linux#1034

Open

Noratrieb reviewed Sep 19, 2023

View reviewed changes

rcvalle added 2 commits October 4, 2023 10:05

Disable CFI for core and std CFI violations

7b45674

Works around rust-lang#115199 by temporarily disabling CFI for core and std CFI violations to allow the user rebuild and use both core and std with CFI enabled using the Cargo build-std feature.

Update documentation for LLVM CFI support

abece4f

Updates documentation for LLVM CFI support with recommended information since the user can now rebuild and use both core and std with CFI enabled using the Cargo build-std feature.

rcvalle force-pushed the rust-cfi-fix-115199 branch from d4ed0a0 to abece4f Compare October 4, 2023 17:14

workingjubilee reviewed Oct 4, 2023

View reviewed changes

workingjubilee approved these changes Oct 4, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 4, 2023

bors merged commit eea2614 into rust-lang:master Oct 4, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 4, 2023

pvdrz mentioned this pull request Oct 6, 2023

Fix divergence from upstream master ferrocene/ferrocene#23

Merged

rcvalle mentioned this pull request Nov 17, 2023

CFI: Add missing use core::ffi::c_int #117790

Merged

rcvalle deleted the rust-cfi-fix-115199 branch April 22, 2024 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable CFI for core and std CFI violations #115200

Disable CFI for core and std CFI violations #115200

rcvalle commented Aug 25, 2023

rustbot commented Aug 25, 2023

ehuss commented Aug 25, 2023

compiler-errors Aug 25, 2023

workingjubilee Aug 25, 2023

workingjubilee Aug 25, 2023 •

edited

Loading

compiler-errors Aug 25, 2023

rcvalle Aug 25, 2023

rcvalle Aug 25, 2023

workingjubilee Aug 25, 2023

This comment has been minimized.

This comment has been minimized.

rcvalle commented Sep 13, 2023

bors commented Sep 14, 2023

maurer commented Sep 15, 2023

rcvalle commented Sep 15, 2023 •

edited

Loading

Noratrieb Sep 19, 2023

bjorn3 Sep 19, 2023

rcvalle commented Oct 4, 2023

workingjubilee left a comment •

edited

Loading

workingjubilee Aug 26, 2023

workingjubilee commented Oct 4, 2023

bors commented Oct 4, 2023

rcvalle commented Oct 4, 2023

bors commented Oct 4, 2023

bors commented Oct 4, 2023

rust-timer commented Oct 4, 2023

		// FIXME: Rust's "try catch" construct (i.e., std::panicking::r#try) use
		// of FnOnce explicitly violates CFI.

	Fig. 13. Build and execution of FIgs. 10–11 with LLVM CFI enabled.
	Fig. 13. Build and execution of Figs. 10–11 with LLVM CFI enabled.

Disable CFI for core and std CFI violations #115200

Disable CFI for core and std CFI violations #115200

Conversation

rcvalle commented Aug 25, 2023

rustbot commented Aug 25, 2023

ehuss commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

rcvalle commented Sep 13, 2023

bors commented Sep 14, 2023

maurer commented Sep 15, 2023

rcvalle commented Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcvalle commented Oct 4, 2023

workingjubilee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Oct 4, 2023

bors commented Oct 4, 2023

rcvalle commented Oct 4, 2023

bors commented Oct 4, 2023

bors commented Oct 4, 2023

rust-timer commented Oct 4, 2023

Overall result: no relevant changes - no action needed

workingjubilee Aug 25, 2023 •

edited

Loading

rcvalle commented Sep 15, 2023 •

edited

Loading

workingjubilee left a comment •

edited

Loading