From f9f2acac1f7335ccb5129f97c963ac937b121b49 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Thu, 10 Apr 2025 13:55:52 +0000 Subject: [PATCH 1/7] cfi: do not transmute function pointers in formatting code --- library/core/src/fmt/mod.rs | 2 +- library/core/src/fmt/rt.rs | 45 +++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/library/core/src/fmt/mod.rs b/library/core/src/fmt/mod.rs index 7ca390941bc..580f95eddce 100644 --- a/library/core/src/fmt/mod.rs +++ b/library/core/src/fmt/mod.rs @@ -7,7 +7,7 @@ use crate::char::{EscapeDebugExtArgs, MAX_LEN_UTF8}; use crate::marker::PhantomData; use crate::num::fmt as numfmt; use crate::ops::Deref; -use crate::{iter, mem, result, str}; +use crate::{iter, result, str}; mod builders; #[cfg(not(no_fp_fmt_parse))] diff --git a/library/core/src/fmt/rt.rs b/library/core/src/fmt/rt.rs index 0459674303d..05e024b134d 100644 --- a/library/core/src/fmt/rt.rs +++ b/library/core/src/fmt/rt.rs @@ -65,61 +65,67 @@ pub struct Argument<'a> { ty: ArgumentType<'a>, } -#[rustc_diagnostic_item = "ArgumentMethods"] -impl Argument<'_> { - #[inline] - const fn new<'a, T>(x: &'a T, f: fn(&T, &mut Formatter<'_>) -> Result) -> Argument<'a> { +macro_rules! argument_new { + ($t:ty, $x:expr, $f:expr) => { Argument { // INVARIANT: this creates an `ArgumentType<'a>` from a `&'a T` and // a `fn(&T, ...)`, so the invariant is maintained. ty: ArgumentType::Placeholder { - value: NonNull::from_ref(x).cast(), + value: NonNull::<$t>::from_ref($x).cast(), // SAFETY: function pointers always have the same layout. - formatter: unsafe { mem::transmute(f) }, + formatter: |ptr: NonNull<()>, fmt: &mut Formatter<'_>| { + let func = $f; + // SAFETY: This is the same type as the `value` field. + let r = unsafe { ptr.cast::<$t>().as_ref() }; + (func)(r, fmt) + }, _lifetime: PhantomData, }, } - } + }; +} +#[rustc_diagnostic_item = "ArgumentMethods"] +impl Argument<'_> { #[inline] pub fn new_display(x: &T) -> Argument<'_> { - Self::new(x, Display::fmt) + argument_new!(T, x, ::fmt) } #[inline] pub fn new_debug(x: &T) -> Argument<'_> { - Self::new(x, Debug::fmt) + argument_new!(T, x, ::fmt) } #[inline] pub fn new_debug_noop(x: &T) -> Argument<'_> { - Self::new(x, |_, _| Ok(())) + argument_new!(T, x, |_: &T, _| Ok(())) } #[inline] pub fn new_octal(x: &T) -> Argument<'_> { - Self::new(x, Octal::fmt) + argument_new!(T, x, ::fmt) } #[inline] pub fn new_lower_hex(x: &T) -> Argument<'_> { - Self::new(x, LowerHex::fmt) + argument_new!(T, x, ::fmt) } #[inline] pub fn new_upper_hex(x: &T) -> Argument<'_> { - Self::new(x, UpperHex::fmt) + argument_new!(T, x, ::fmt) } #[inline] pub fn new_pointer(x: &T) -> Argument<'_> { - Self::new(x, Pointer::fmt) + argument_new!(T, x, ::fmt) } #[inline] pub fn new_binary(x: &T) -> Argument<'_> { - Self::new(x, Binary::fmt) + argument_new!(T, x, ::fmt) } #[inline] pub fn new_lower_exp(x: &T) -> Argument<'_> { - Self::new(x, LowerExp::fmt) + argument_new!(T, x, ::fmt) } #[inline] pub fn new_upper_exp(x: &T) -> Argument<'_> { - Self::new(x, UpperExp::fmt) + argument_new!(T, x, ::fmt) } #[inline] #[track_caller] @@ -135,11 +141,6 @@ impl Argument<'_> { /// # Safety /// /// This argument must actually be a placeholder argument. - /// - // FIXME: Transmuting formatter in new and indirectly branching to/calling - // it here is an explicit CFI violation. - #[allow(inline_no_sanitize)] - #[no_sanitize(cfi, kcfi)] #[inline] pub(super) unsafe fn fmt(&self, f: &mut Formatter<'_>) -> Result { match self.ty { From 997ec4982b6be6c4d2bffe7dcb0df420d78482de Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Thu, 10 Apr 2025 14:32:59 +0000 Subject: [PATCH 2/7] Update code coverage map --- tests/coverage/closure.cov-map | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/tests/coverage/closure.cov-map b/tests/coverage/closure.cov-map index 2d784ba09b6..640f9895684 100644 --- a/tests/coverage/closure.cov-map +++ b/tests/coverage/closure.cov-map @@ -140,19 +140,17 @@ Number of file 0 mappings: 6 - Code(Counter(0)) at (prev + 2, 9) to (start + 0, 10) Highest counter ID seen: c1 -Function name: closure::main::{closure#18} -Raw bytes (26): 0x[01, 01, 01, 01, 05, 04, 01, 19, 0d, 02, 1c, 05, 02, 1d, 02, 12, 02, 02, 11, 00, 12, 01, 01, 11, 01, 0e] +Function name: closure::main::{closure#18} (unused) +Raw bytes (24): 0x[01, 01, 00, 04, 00, 19, 0d, 02, 1c, 00, 02, 1d, 02, 12, 00, 02, 11, 00, 12, 00, 01, 11, 01, 0e] Number of files: 1 - file 0 => global file 1 -Number of expressions: 1 -- expression 0 operands: lhs = Counter(0), rhs = Counter(1) +Number of expressions: 0 Number of file 0 mappings: 4 -- Code(Counter(0)) at (prev + 25, 13) to (start + 2, 28) -- Code(Counter(1)) at (prev + 2, 29) to (start + 2, 18) -- Code(Expression(0, Sub)) at (prev + 2, 17) to (start + 0, 18) - = (c0 - c1) -- Code(Counter(0)) at (prev + 1, 17) to (start + 1, 14) -Highest counter ID seen: c1 +- Code(Zero) at (prev + 25, 13) to (start + 2, 28) +- Code(Zero) at (prev + 2, 29) to (start + 2, 18) +- Code(Zero) at (prev + 2, 17) to (start + 0, 18) +- Code(Zero) at (prev + 1, 17) to (start + 1, 14) +Highest counter ID seen: (none) Function name: closure::main::{closure#19} Raw bytes (26): 0x[01, 01, 01, 01, 05, 04, 01, 43, 0d, 02, 1c, 05, 02, 1d, 02, 12, 02, 02, 11, 00, 12, 01, 01, 11, 01, 0e] From 17d412439ad883bc52730cad194088246d557752 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Fri, 11 Apr 2025 17:55:25 +0200 Subject: [PATCH 3/7] Update library/core/src/fmt/rt.rs --- library/core/src/fmt/rt.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/library/core/src/fmt/rt.rs b/library/core/src/fmt/rt.rs index 05e024b134d..702aa7e3436 100644 --- a/library/core/src/fmt/rt.rs +++ b/library/core/src/fmt/rt.rs @@ -72,7 +72,6 @@ macro_rules! argument_new { // a `fn(&T, ...)`, so the invariant is maintained. ty: ArgumentType::Placeholder { value: NonNull::<$t>::from_ref($x).cast(), - // SAFETY: function pointers always have the same layout. formatter: |ptr: NonNull<()>, fmt: &mut Formatter<'_>| { let func = $f; // SAFETY: This is the same type as the `value` field. From 6c6a39e6bfd10bccbf6ca99479655a9ef8d87b39 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Sat, 12 Apr 2025 10:49:39 +0000 Subject: [PATCH 4/7] cfg(kcfi) --- library/core/src/fmt/mod.rs | 2 +- library/core/src/fmt/rt.rs | 7 +++++++ tests/coverage/closure.cov-map | 18 ++++++++++-------- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/library/core/src/fmt/mod.rs b/library/core/src/fmt/mod.rs index 580f95eddce..7ca390941bc 100644 --- a/library/core/src/fmt/mod.rs +++ b/library/core/src/fmt/mod.rs @@ -7,7 +7,7 @@ use crate::char::{EscapeDebugExtArgs, MAX_LEN_UTF8}; use crate::marker::PhantomData; use crate::num::fmt as numfmt; use crate::ops::Deref; -use crate::{iter, result, str}; +use crate::{iter, mem, result, str}; mod builders; #[cfg(not(no_fp_fmt_parse))] diff --git a/library/core/src/fmt/rt.rs b/library/core/src/fmt/rt.rs index 702aa7e3436..c36feff0b62 100644 --- a/library/core/src/fmt/rt.rs +++ b/library/core/src/fmt/rt.rs @@ -72,6 +72,13 @@ macro_rules! argument_new { // a `fn(&T, ...)`, so the invariant is maintained. ty: ArgumentType::Placeholder { value: NonNull::<$t>::from_ref($x).cast(), + #[cfg(not(any(sanitize = "cfi", sanitize = "kcfi")))] + formatter: { + let f: fn(&$t, &mut Formatter<'_>) -> Result = $f; + // SAFETY: This is only called with `value`, which has the right type. + unsafe { mem::transmute(f) } + }, + #[cfg(any(sanitize = "cfi", sanitize = "kcfi"))] formatter: |ptr: NonNull<()>, fmt: &mut Formatter<'_>| { let func = $f; // SAFETY: This is the same type as the `value` field. diff --git a/tests/coverage/closure.cov-map b/tests/coverage/closure.cov-map index 640f9895684..2d784ba09b6 100644 --- a/tests/coverage/closure.cov-map +++ b/tests/coverage/closure.cov-map @@ -140,17 +140,19 @@ Number of file 0 mappings: 6 - Code(Counter(0)) at (prev + 2, 9) to (start + 0, 10) Highest counter ID seen: c1 -Function name: closure::main::{closure#18} (unused) -Raw bytes (24): 0x[01, 01, 00, 04, 00, 19, 0d, 02, 1c, 00, 02, 1d, 02, 12, 00, 02, 11, 00, 12, 00, 01, 11, 01, 0e] +Function name: closure::main::{closure#18} +Raw bytes (26): 0x[01, 01, 01, 01, 05, 04, 01, 19, 0d, 02, 1c, 05, 02, 1d, 02, 12, 02, 02, 11, 00, 12, 01, 01, 11, 01, 0e] Number of files: 1 - file 0 => global file 1 -Number of expressions: 0 +Number of expressions: 1 +- expression 0 operands: lhs = Counter(0), rhs = Counter(1) Number of file 0 mappings: 4 -- Code(Zero) at (prev + 25, 13) to (start + 2, 28) -- Code(Zero) at (prev + 2, 29) to (start + 2, 18) -- Code(Zero) at (prev + 2, 17) to (start + 0, 18) -- Code(Zero) at (prev + 1, 17) to (start + 1, 14) -Highest counter ID seen: (none) +- Code(Counter(0)) at (prev + 25, 13) to (start + 2, 28) +- Code(Counter(1)) at (prev + 2, 29) to (start + 2, 18) +- Code(Expression(0, Sub)) at (prev + 2, 17) to (start + 0, 18) + = (c0 - c1) +- Code(Counter(0)) at (prev + 1, 17) to (start + 1, 14) +Highest counter ID seen: c1 Function name: closure::main::{closure#19} Raw bytes (26): 0x[01, 01, 01, 01, 05, 04, 01, 43, 0d, 02, 1c, 05, 02, 1d, 02, 12, 02, 02, 11, 00, 12, 01, 01, 11, 01, 0e] From 2009ca6d88a363b84b4733e253835fe13ec96540 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Mon, 14 Apr 2025 13:54:14 +0000 Subject: [PATCH 5/7] Remove #![feature(no_sanitize)] --- library/core/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/library/core/src/lib.rs b/library/core/src/lib.rs index dc06aa4c38d..c2d65f07364 100644 --- a/library/core/src/lib.rs +++ b/library/core/src/lib.rs @@ -169,7 +169,6 @@ #![feature(negative_impls)] #![feature(never_type)] #![feature(no_core)] -#![feature(no_sanitize)] #![feature(optimize_attribute)] #![feature(prelude_import)] #![feature(repr_simd)] From 1cbdfab75d6dbf9c383ece8fe2e2642e5db2f2e7 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Mon, 14 Apr 2025 13:54:49 +0000 Subject: [PATCH 6/7] Use full path for core::mem::transmute Suggested-by: Tamir Duberstein Signed-off-by: Alice Ryhl --- library/core/src/fmt/mod.rs | 2 +- library/core/src/fmt/rt.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/fmt/mod.rs b/library/core/src/fmt/mod.rs index 7ca390941bc..580f95eddce 100644 --- a/library/core/src/fmt/mod.rs +++ b/library/core/src/fmt/mod.rs @@ -7,7 +7,7 @@ use crate::char::{EscapeDebugExtArgs, MAX_LEN_UTF8}; use crate::marker::PhantomData; use crate::num::fmt as numfmt; use crate::ops::Deref; -use crate::{iter, mem, result, str}; +use crate::{iter, result, str}; mod builders; #[cfg(not(no_fp_fmt_parse))] diff --git a/library/core/src/fmt/rt.rs b/library/core/src/fmt/rt.rs index c36feff0b62..b9cae8645cd 100644 --- a/library/core/src/fmt/rt.rs +++ b/library/core/src/fmt/rt.rs @@ -76,7 +76,7 @@ macro_rules! argument_new { formatter: { let f: fn(&$t, &mut Formatter<'_>) -> Result = $f; // SAFETY: This is only called with `value`, which has the right type. - unsafe { mem::transmute(f) } + unsafe { core::mem::transmute(f) } }, #[cfg(any(sanitize = "cfi", sanitize = "kcfi"))] formatter: |ptr: NonNull<()>, fmt: &mut Formatter<'_>| { From 6513df96525704d3c40f16faf2672adaaa3186ac Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Mon, 14 Apr 2025 06:52:32 +0000 Subject: [PATCH 7/7] Add comment Co-authored-by: Ralf Jung --- library/core/src/fmt/rt.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/library/core/src/fmt/rt.rs b/library/core/src/fmt/rt.rs index b9cae8645cd..adcfdd309b7 100644 --- a/library/core/src/fmt/rt.rs +++ b/library/core/src/fmt/rt.rs @@ -72,6 +72,25 @@ macro_rules! argument_new { // a `fn(&T, ...)`, so the invariant is maintained. ty: ArgumentType::Placeholder { value: NonNull::<$t>::from_ref($x).cast(), + // The Rust ABI considers all pointers to be equivalent, so transmuting a fn(&T) to + // fn(NonNull<()>) and calling it with a NonNull<()> that points at a T is allowed. + // However, the CFI sanitizer does not allow this, and triggers a crash when it + // happens. + // + // To avoid this crash, we use a helper function when CFI is enabled. To avoid the + // cost of this helper function (mainly code-size) when it is not needed, we + // transmute the function pointer otherwise. + // + // This is similar to what the Rust compiler does internally with vtables when KCFI + // is enabled, where it generates trampoline functions that only serve to adjust the + // expected type of the argument. `ArgumentType::Placeholder` is a bit like a + // manually constructed trait object, so it is not surprising that the same approach + // has to be applied here as well. + // + // It is still considered problematic (from the Rust side) that CFI rejects entirely + // legal Rust programs, so we do not consider anything done here a stable guarantee, + // but meanwhile we carry this work-around to keep Rust compatible with CFI and + // KCFI. #[cfg(not(any(sanitize = "cfi", sanitize = "kcfi")))] formatter: { let f: fn(&$t, &mut Formatter<'_>) -> Result = $f;