From 7c7b22e62cd3aa34ef60ec98b145258caa55261f Mon Sep 17 00:00:00 2001 From: Ramon de C Valle Date: Wed, 3 May 2023 22:22:24 +0000 Subject: [PATCH] CFI: Fix SIGILL reached via trait objects Fix #106547 by transforming the concrete self into a reference to a trait object before emitting type metadata identifiers for trait methods. --- compiler/rustc_codegen_llvm/src/callee.rs | 4 +- .../src/coverageinfo/mod.rs | 1 + compiler/rustc_codegen_llvm/src/declare.rs | 64 ++++++++++++----- compiler/rustc_codegen_llvm/src/intrinsic.rs | 2 +- compiler/rustc_codegen_llvm/src/mono_item.rs | 2 +- compiler/rustc_symbol_mangling/src/typeid.rs | 24 ++++++- .../src/typeid/typeid_itanium_cxx_abi.rs | 57 ++++++++++++++- compiler/rustc_target/src/abi/call/mod.rs | 8 +-- ...er-cfi-emit-type-metadata-trait-objects.rs | 44 ++++++++++++ ...r-kcfi-emit-type-metadata-trait-objects.rs | 69 +++++++++++++++++++ 10 files changed, 247 insertions(+), 28 deletions(-) create mode 100644 tests/codegen/sanitizer-cfi-emit-type-metadata-trait-objects.rs create mode 100644 tests/codegen/sanitizer-kcfi-emit-type-metadata-trait-objects.rs diff --git a/compiler/rustc_codegen_llvm/src/callee.rs b/compiler/rustc_codegen_llvm/src/callee.rs index 30a0cf1d01914..4b9ca2e7d19c3 100644 --- a/compiler/rustc_codegen_llvm/src/callee.rs +++ b/compiler/rustc_codegen_llvm/src/callee.rs @@ -94,11 +94,11 @@ pub fn get_fn<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>) -> // LLVM will prefix the name with `__imp_`. Ideally, we'd like the // existing logic below to set the Storage Class, but it has an // exemption for MinGW for backwards compatability. - let llfn = cx.declare_fn(&common::i686_decorated_name(&dllimport, common::is_mingw_gnu_toolchain(&tcx.sess.target), true), fn_abi); + let llfn = cx.declare_fn(&common::i686_decorated_name(&dllimport, common::is_mingw_gnu_toolchain(&tcx.sess.target), true), fn_abi, Some(instance)); unsafe { llvm::LLVMSetDLLStorageClass(llfn, llvm::DLLStorageClass::DllImport); } llfn } else { - cx.declare_fn(sym, fn_abi) + cx.declare_fn(sym, fn_abi, Some(instance)) }; debug!("get_fn: not casting pointer!"); diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 3dc0ac03312e9..cd261293e9b2d 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -207,6 +207,7 @@ fn declare_unused_fn<'tcx>(cx: &CodegenCx<'_, 'tcx>, def_id: DefId) -> Instance< )), ty::List::empty(), ), + None, ); llvm::set_linkage(llfn, llvm::Linkage::PrivateLinkage); diff --git a/compiler/rustc_codegen_llvm/src/declare.rs b/compiler/rustc_codegen_llvm/src/declare.rs index cc2a5d158be82..164b12cf8d411 100644 --- a/compiler/rustc_codegen_llvm/src/declare.rs +++ b/compiler/rustc_codegen_llvm/src/declare.rs @@ -19,8 +19,11 @@ use crate::llvm::AttributePlace::Function; use crate::type_::Type; use crate::value::Value; use rustc_codegen_ssa::traits::TypeMembershipMethods; -use rustc_middle::ty::Ty; -use rustc_symbol_mangling::typeid::{kcfi_typeid_for_fnabi, typeid_for_fnabi, TypeIdOptions}; +use rustc_middle::ty::{Instance, Ty}; +use rustc_symbol_mangling::typeid::{ + kcfi_typeid_for_fnabi, kcfi_typeid_for_instance, typeid_for_fnabi, typeid_for_instance, + TypeIdOptions, +}; use smallvec::SmallVec; /// Declare a function. @@ -116,7 +119,12 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> { /// /// If there’s a value with the same name already declared, the function will /// update the declaration and return existing Value instead. - pub fn declare_fn(&self, name: &str, fn_abi: &FnAbi<'tcx, Ty<'tcx>>) -> &'ll Value { + pub fn declare_fn( + &self, + name: &str, + fn_abi: &FnAbi<'tcx, Ty<'tcx>>, + instance: Option>, + ) -> &'ll Value { debug!("declare_rust_fn(name={:?}, fn_abi={:?})", name, fn_abi); // Function addresses in Rust are never significant, allowing functions to @@ -132,18 +140,35 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> { fn_abi.apply_attrs_llfn(self, llfn); if self.tcx.sess.is_sanitizer_cfi_enabled() { - let typeid = typeid_for_fnabi(self.tcx, fn_abi, TypeIdOptions::empty()); - self.set_type_metadata(llfn, typeid); - let typeid = typeid_for_fnabi(self.tcx, fn_abi, TypeIdOptions::GENERALIZE_POINTERS); - self.add_type_metadata(llfn, typeid); - let typeid = typeid_for_fnabi(self.tcx, fn_abi, TypeIdOptions::NORMALIZE_INTEGERS); - self.add_type_metadata(llfn, typeid); - let typeid = typeid_for_fnabi( - self.tcx, - fn_abi, - TypeIdOptions::GENERALIZE_POINTERS | TypeIdOptions::NORMALIZE_INTEGERS, - ); - self.add_type_metadata(llfn, typeid); + if let Some(instance) = instance { + let typeid = typeid_for_instance(self.tcx, &instance, TypeIdOptions::empty()); + self.set_type_metadata(llfn, typeid); + let typeid = + typeid_for_instance(self.tcx, &instance, TypeIdOptions::GENERALIZE_POINTERS); + self.add_type_metadata(llfn, typeid); + let typeid = + typeid_for_instance(self.tcx, &instance, TypeIdOptions::NORMALIZE_INTEGERS); + self.add_type_metadata(llfn, typeid); + let typeid = typeid_for_instance( + self.tcx, + &instance, + TypeIdOptions::GENERALIZE_POINTERS | TypeIdOptions::NORMALIZE_INTEGERS, + ); + self.add_type_metadata(llfn, typeid); + } else { + let typeid = typeid_for_fnabi(self.tcx, fn_abi, TypeIdOptions::empty()); + self.set_type_metadata(llfn, typeid); + let typeid = typeid_for_fnabi(self.tcx, fn_abi, TypeIdOptions::GENERALIZE_POINTERS); + self.add_type_metadata(llfn, typeid); + let typeid = typeid_for_fnabi(self.tcx, fn_abi, TypeIdOptions::NORMALIZE_INTEGERS); + self.add_type_metadata(llfn, typeid); + let typeid = typeid_for_fnabi( + self.tcx, + fn_abi, + TypeIdOptions::GENERALIZE_POINTERS | TypeIdOptions::NORMALIZE_INTEGERS, + ); + self.add_type_metadata(llfn, typeid); + } } if self.tcx.sess.is_sanitizer_kcfi_enabled() { @@ -156,8 +181,13 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> { options.insert(TypeIdOptions::NORMALIZE_INTEGERS); } - let kcfi_typeid = kcfi_typeid_for_fnabi(self.tcx, fn_abi, options); - self.set_kcfi_type_metadata(llfn, kcfi_typeid); + if let Some(instance) = instance { + let kcfi_typeid = kcfi_typeid_for_instance(self.tcx, &instance, options); + self.set_kcfi_type_metadata(llfn, kcfi_typeid); + } else { + let kcfi_typeid = kcfi_typeid_for_fnabi(self.tcx, fn_abi, options); + self.set_kcfi_type_metadata(llfn, kcfi_typeid); + } } llfn diff --git a/compiler/rustc_codegen_llvm/src/intrinsic.rs b/compiler/rustc_codegen_llvm/src/intrinsic.rs index 00d1796f210bb..4e28034a8507b 100644 --- a/compiler/rustc_codegen_llvm/src/intrinsic.rs +++ b/compiler/rustc_codegen_llvm/src/intrinsic.rs @@ -772,7 +772,7 @@ fn gen_fn<'ll, 'tcx>( ) -> (&'ll Type, &'ll Value) { let fn_abi = cx.fn_abi_of_fn_ptr(rust_fn_sig, ty::List::empty()); let llty = fn_abi.llvm_type(cx); - let llfn = cx.declare_fn(name, fn_abi); + let llfn = cx.declare_fn(name, fn_abi, None); cx.set_frame_pointer_type(llfn); cx.apply_target_cpu_attr(llfn); // FIXME(eddyb) find a nicer way to do this. diff --git a/compiler/rustc_codegen_llvm/src/mono_item.rs b/compiler/rustc_codegen_llvm/src/mono_item.rs index 59bdc60830f80..e8f8c321510a2 100644 --- a/compiler/rustc_codegen_llvm/src/mono_item.rs +++ b/compiler/rustc_codegen_llvm/src/mono_item.rs @@ -51,7 +51,7 @@ impl<'tcx> PreDefineMethods<'tcx> for CodegenCx<'_, 'tcx> { assert!(!instance.substs.has_infer()); let fn_abi = self.fn_abi_of_instance(instance, ty::List::empty()); - let lldecl = self.declare_fn(symbol_name, fn_abi); + let lldecl = self.declare_fn(symbol_name, fn_abi, Some(instance)); unsafe { llvm::LLVMRustSetLinkage(lldecl, base::linkage_to_llvm(linkage)) }; let attrs = self.tcx.codegen_fn_attrs(instance.def_id()); base::set_link_section(lldecl, attrs); diff --git a/compiler/rustc_symbol_mangling/src/typeid.rs b/compiler/rustc_symbol_mangling/src/typeid.rs index 81dbff9ea4e40..cda16e3a3f566 100644 --- a/compiler/rustc_symbol_mangling/src/typeid.rs +++ b/compiler/rustc_symbol_mangling/src/typeid.rs @@ -4,7 +4,7 @@ /// For more information about LLVM CFI and cross-language LLVM CFI support for the Rust compiler, /// see design document in the tracking issue #89653. use bitflags::bitflags; -use rustc_middle::ty::{FnSig, Ty, TyCtxt}; +use rustc_middle::ty::{FnSig, Instance, Ty, TyCtxt}; use rustc_target::abi::call::FnAbi; use std::hash::Hasher; use twox_hash::XxHash64; @@ -38,6 +38,15 @@ pub fn typeid_for_fnsig<'tcx>( typeid_itanium_cxx_abi::typeid_for_fnsig(tcx, fn_sig, options) } +/// Returns a type metadata identifier for the specified Instance. +pub fn typeid_for_instance<'tcx>( + tcx: TyCtxt<'tcx>, + instance: &Instance<'tcx>, + options: TypeIdOptions, +) -> String { + typeid_itanium_cxx_abi::typeid_for_instance(tcx, instance, options) +} + /// Returns a KCFI type metadata identifier for the specified FnAbi. pub fn kcfi_typeid_for_fnabi<'tcx>( tcx: TyCtxt<'tcx>, @@ -63,3 +72,16 @@ pub fn kcfi_typeid_for_fnsig<'tcx>( hash.write(typeid_itanium_cxx_abi::typeid_for_fnsig(tcx, fn_sig, options).as_bytes()); hash.finish() as u32 } + +/// Returns a KCFI type metadata identifier for the specified Instance. +pub fn kcfi_typeid_for_instance<'tcx>( + tcx: TyCtxt<'tcx>, + instance: &Instance<'tcx>, + options: TypeIdOptions, +) -> u32 { + // A KCFI type metadata identifier is a 32-bit constant produced by taking the lower half of the + // xxHash64 of the type metadata identifier. (See llvm/llvm-project@cff5bef.) + let mut hash: XxHash64 = Default::default(); + hash.write(typeid_itanium_cxx_abi::typeid_for_instance(tcx, instance, options).as_bytes()); + hash.finish() as u32 +} diff --git a/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs b/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs index 5310ef26da768..c281aa7e83a85 100644 --- a/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs +++ b/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs @@ -14,8 +14,8 @@ use rustc_errors::DiagnosticMessage; use rustc_hir as hir; use rustc_middle::ty::subst::{GenericArg, GenericArgKind, SubstsRef}; use rustc_middle::ty::{ - self, Const, ExistentialPredicate, FloatTy, FnSig, IntTy, List, Region, RegionKind, TermKind, - Ty, TyCtxt, UintTy, + self, Const, ExistentialPredicate, FloatTy, FnSig, Instance, IntTy, List, Region, RegionKind, + TermKind, Ty, TyCtxt, UintTy, }; use rustc_span::def_id::DefId; use rustc_span::sym; @@ -1010,3 +1010,56 @@ pub fn typeid_for_fnsig<'tcx>( typeid } + +/// Returns a type metadata identifier for the specified Instance using the Itanium C++ ABI with +/// vendor extended type qualifiers and types for Rust types that are not used at the FFI boundary. +pub fn typeid_for_instance<'tcx>( + tcx: TyCtxt<'tcx>, + instance: &Instance<'tcx>, + options: TypeIdOptions, +) -> String { + let fn_abi = tcx + .fn_abi_of_instance(tcx.param_env(instance.def_id()).and((*instance, ty::List::empty()))) + .unwrap_or_else(|instance| { + bug!("typeid_for_instance: couldn't get fn_abi of instance {:?}", instance) + }); + + // If this instance is a method and self is a reference, get the impl it belongs to + let impl_def_id = tcx.impl_of_method(instance.def_id()); + if impl_def_id.is_some() && !fn_abi.args.is_empty() && fn_abi.args[0].layout.ty.is_ref() { + // If this impl is not an inherent impl, get the trait it implements + if let Some(trait_ref) = tcx.impl_trait_ref(impl_def_id.unwrap()) { + // Transform the concrete self into a reference to a trait object + let existential_predicate = trait_ref.map_bound(|trait_ref| { + ty::ExistentialPredicate::Trait(ty::ExistentialTraitRef::erase_self_ty( + tcx, trait_ref, + )) + }); + let existential_predicates = tcx.mk_poly_existential_predicates(&[ty::Binder::dummy( + existential_predicate.skip_binder(), + )]); + // Is the concrete self mutable? + let self_ty = if fn_abi.args[0].layout.ty.is_mutable_ptr() { + tcx.mk_mut_ref( + tcx.lifetimes.re_erased, + tcx.mk_dynamic(existential_predicates, tcx.lifetimes.re_erased, ty::Dyn), + ) + } else { + tcx.mk_imm_ref( + tcx.lifetimes.re_erased, + tcx.mk_dynamic(existential_predicates, tcx.lifetimes.re_erased, ty::Dyn), + ) + }; + + // Replace the concrete self in an fn_abi clone by the reference to a trait object + let mut fn_abi = fn_abi.clone(); + // HACK(rcvalle): It is okay to not replace or update the entire ArgAbi here because the + // other fields are never used. + fn_abi.args[0].layout.ty = self_ty; + + return typeid_for_fnabi(tcx, &fn_abi, options); + } + } + + typeid_for_fnabi(tcx, &fn_abi, options) +} diff --git a/compiler/rustc_target/src/abi/call/mod.rs b/compiler/rustc_target/src/abi/call/mod.rs index 57011aa8a1474..0aacc8ba5de95 100644 --- a/compiler/rustc_target/src/abi/call/mod.rs +++ b/compiler/rustc_target/src/abi/call/mod.rs @@ -28,7 +28,7 @@ mod x86; mod x86_64; mod x86_win64; -#[derive(PartialEq, Eq, Hash, Debug, HashStable_Generic)] +#[derive(Clone, PartialEq, Eq, Hash, Debug, HashStable_Generic)] pub enum PassMode { /// Ignore the argument. /// @@ -211,7 +211,7 @@ impl Uniform { } } -#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, HashStable_Generic)] +#[derive(Clone, PartialEq, Eq, Hash, Debug, HashStable_Generic)] pub struct CastTarget { pub prefix: [Option; 8], pub rest: Uniform, @@ -458,7 +458,7 @@ impl<'a, Ty> TyAndLayout<'a, Ty> { /// Information about how to pass an argument to, /// or return a value from, a function, under some ABI. -#[derive(PartialEq, Eq, Hash, Debug, HashStable_Generic)] +#[derive(Clone, PartialEq, Eq, Hash, Debug, HashStable_Generic)] pub struct ArgAbi<'a, Ty> { pub layout: TyAndLayout<'a, Ty>, pub mode: PassMode, @@ -605,7 +605,7 @@ pub enum Conv { /// /// I will do my best to describe this structure, but these /// comments are reverse-engineered and may be inaccurate. -NDM -#[derive(PartialEq, Eq, Hash, Debug, HashStable_Generic)] +#[derive(Clone, PartialEq, Eq, Hash, Debug, HashStable_Generic)] pub struct FnAbi<'a, Ty> { /// The LLVM types of each argument. pub args: Box<[ArgAbi<'a, Ty>]>, diff --git a/tests/codegen/sanitizer-cfi-emit-type-metadata-trait-objects.rs b/tests/codegen/sanitizer-cfi-emit-type-metadata-trait-objects.rs new file mode 100644 index 0000000000000..ab5dcec7936ca --- /dev/null +++ b/tests/codegen/sanitizer-cfi-emit-type-metadata-trait-objects.rs @@ -0,0 +1,44 @@ +// Verifies that type metadata identifiers for trait objects are emitted correctly. +// +// needs-sanitizer-cfi +// compile-flags: -Clto -Cno-prepopulate-passes -Ctarget-feature=-crt-static -Zsanitizer=cfi + +#![crate_type="lib"] + +trait Trait1 { + fn foo(&self); +} + +struct Type1; + +impl Trait1 for Type1 { + fn foo(&self) { + } +} + +pub fn foo() { + let a = Type1; + a.foo(); + // CHECK-LABEL: define{{.*}}foo{{.*}}!type !{{[0-9]+}} + // CHECK: call ::foo +} + +pub fn bar() { + let a = Type1; + let b = &a as &dyn Trait1; + b.foo(); + // CHECK-LABEL: define{{.*}}bar{{.*}}!type !{{[0-9]+}} + // CHECK: call i1 @llvm.type.test({{i8\*|ptr}} {{%f|%0|%1}}, metadata !"[[TYPE1:[[:print:]]+]]") +} + +pub fn baz() { + let a = Type1; + let b = &a as &dyn Trait1; + a.foo(); + b.foo(); + // CHECK-LABEL: define{{.*}}baz{{.*}}!type !{{[0-9]+}} + // CHECK: call ::foo + // CHECK: call i1 @llvm.type.test({{i8\*|ptr}} {{%f|%0|%1}}, metadata !"[[TYPE1:[[:print:]]+]]") +} + +// CHECK: !{{[0-9]+}} = !{i64 0, !"[[TYPE1]]"} diff --git a/tests/codegen/sanitizer-kcfi-emit-type-metadata-trait-objects.rs b/tests/codegen/sanitizer-kcfi-emit-type-metadata-trait-objects.rs new file mode 100644 index 0000000000000..81e0d9344f7e4 --- /dev/null +++ b/tests/codegen/sanitizer-kcfi-emit-type-metadata-trait-objects.rs @@ -0,0 +1,69 @@ +// Verifies that type metadata identifiers for trait objects are emitted correctly. +// +// revisions: aarch64 x86_64 +// [aarch64] compile-flags: --target aarch64-unknown-none +// [aarch64] needs-llvm-components: aarch64 +// [x86_64] compile-flags: --target x86_64-unknown-none +// [x86_64] needs-llvm-components: +// compile-flags: -Cno-prepopulate-passes -Zsanitizer=kcfi -Copt-level=0 + +#![crate_type="lib"] +#![feature(arbitrary_self_types, no_core, lang_items)] +#![no_core] + +#[lang="sized"] +trait Sized { } +#[lang="copy"] +trait Copy { } +#[lang="receiver"] +trait Receiver { } +#[lang="dispatch_from_dyn"] +trait DispatchFromDyn { } +impl<'a, T: ?Sized + Unsize, U: ?Sized> DispatchFromDyn<&'a U> for &'a T {} +#[lang = "unsize"] +trait Unsize { } +#[lang = "coerce_unsized"] +pub trait CoerceUnsized { } +impl<'a, 'b: 'a, T: ?Sized + Unsize, U: ?Sized> CoerceUnsized<&'a U> for &'b T {} +#[lang="freeze"] +trait Freeze { } +#[lang="drop_in_place"] +fn drop_in_place_fn() { } + +trait Trait1 { + fn foo(&self); +} + +struct Type1; + +impl Trait1 for Type1 { + fn foo(&self) { + } +} + +pub fn foo() { + let a = Type1; + a.foo(); + // CHECK-LABEL: define{{.*}}foo{{.*}}!{{|kcfi_type}} !{{[0-9]+}} + // CHECK: call ::foo +} + +pub fn bar() { + let a = Type1; + let b = &a as &dyn Trait1; + b.foo(); + // CHECK-LABEL: define{{.*}}bar{{.*}}!{{|kcfi_type}} !{{[0-9]+}} + // CHECK: call void %0({{\{\}\*|ptr}} align 1 {{%b\.0|%_1}}){{.*}}[ "kcfi"(i32 [[TYPE1:[[:print:]]+]]) ] +} + +pub fn baz() { + let a = Type1; + let b = &a as &dyn Trait1; + a.foo(); + b.foo(); + // CHECK-LABEL: define{{.*}}baz{{.*}}!{{|kcfi_type}} !{{[0-9]+}} + // CHECK: call ::foo + // CHECK: call void %0({{\{\}\*|ptr}} align 1 {{%b\.0|%_1}}){{.*}}[ "kcfi"(i32 [[TYPE1:[[:print:]]+]]) ] +} + +// CHECK: !{{[0-9]+}} = !{i32 [[TYPE1]]}