diff --git a/README.md b/README.md index dead4931f2..4452442cd4 100644 --- a/README.md +++ b/README.md @@ -419,11 +419,9 @@ to Miri failing to detect cases of undefined behavior in a program. Finally, the flag is **unsound** in the sense that Miri stops tracking details such as initialization and provenance on memory shared with native code, so it is easily possible to write code that has UB which is missed by Miri. -* `-Zmiri-force-old-native-lib-mode` disables the WIP improved native code access tracking. If for - whatever reason enabling native calls leads to odd behaviours or causes Miri to panic, disabling - the tracer *might* fix this. This will likely be removed once the tracer has been adequately - battle-tested. Note that this flag is only meaningful on Linux systems; other Unixes (currently) - exclusively use the old native-lib code. +* `-Zmiri-native-lib-enable-tracing` enables the WIP detailed tracing mode for invoking native code. + Note that this flag is only meaningful on Linux systems; other Unixes (currently) do not support + tracing mode. * `-Zmiri-measureme=` enables `measureme` profiling for the interpreted program. This can be used to find which parts of your program are executing slowly under Miri. The profile is written out to a file inside a directory called ``, and can be processed diff --git a/src/alloc/isolated_alloc.rs b/src/alloc/isolated_alloc.rs index c3ea7270e8..a7bb9b4da7 100644 --- a/src/alloc/isolated_alloc.rs +++ b/src/alloc/isolated_alloc.rs @@ -305,12 +305,12 @@ impl IsolatedAlloc { /// Returns a vector of page addresses managed by the allocator. pub fn pages(&self) -> Vec { let mut pages: Vec = - self.page_ptrs.clone().into_iter().map(|p| p.expose_provenance().get()).collect(); - self.huge_ptrs.iter().for_each(|(ptr, size)| { + self.page_ptrs.iter().map(|p| p.expose_provenance().get()).collect(); + for (ptr, size) in self.huge_ptrs.iter() { for i in 0..size / self.page_size { pages.push(ptr.expose_provenance().get().strict_add(i * self.page_size)); } - }); + } pages } diff --git a/src/bin/miri.rs b/src/bin/miri.rs index 2e82dbee34..b80327d9d1 100644 --- a/src/bin/miri.rs +++ b/src/bin/miri.rs @@ -228,7 +228,7 @@ impl rustc_driver::Callbacks for MiriCompilerCalls { let return_code = miri::eval_entry(tcx, entry_def_id, entry_type, &config, None) .unwrap_or_else(|| { #[cfg(target_os = "linux")] - miri::register_retcode_sv(rustc_driver::EXIT_FAILURE); + miri::native_lib::register_retcode_sv(rustc_driver::EXIT_FAILURE); tcx.dcx().abort_if_errors(); rustc_driver::EXIT_FAILURE }); @@ -724,8 +724,8 @@ fn main() { } else { show_error!("-Zmiri-native-lib `{}` does not exist", filename); } - } else if arg == "-Zmiri-force-old-native-lib-mode" { - miri_config.force_old_native_lib = true; + } else if arg == "-Zmiri-native-lib-enable-tracing" { + miri_config.native_lib_enable_tracing = true; } else if let Some(param) = arg.strip_prefix("-Zmiri-num-cpus=") { let num_cpus = param .parse::() @@ -797,14 +797,14 @@ fn main() { debug!("rustc arguments: {:?}", rustc_args); debug!("crate arguments: {:?}", miri_config.args); #[cfg(target_os = "linux")] - if !miri_config.native_lib.is_empty() && !miri_config.force_old_native_lib { + if !miri_config.native_lib.is_empty() && miri_config.native_lib_enable_tracing { // FIXME: This should display a diagnostic / warning on error // SAFETY: If any other threads exist at this point (namely for the ctrlc // handler), they will not interact with anything on the main rustc/Miri // thread in an async-signal-unsafe way such as by accessing shared // semaphores, etc.; the handler only calls `sleep()` and `exit()`, which // are async-signal-safe, as is accessing atomics - let _ = unsafe { miri::init_sv() }; + let _ = unsafe { miri::native_lib::init_sv() }; } run_compiler_and_exit( &rustc_args, diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 095f4ba8c8..9ecbd31c5b 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -132,8 +132,9 @@ pub enum NonHaltingDiagnostic { Int2Ptr { details: bool, }, - NativeCallSharedMem, - NativeCallNoTrace, + NativeCallSharedMem { + tracing: bool, + }, WeakMemoryOutdatedLoad { ptr: Pointer, }, @@ -628,10 +629,8 @@ impl<'tcx> MiriMachine<'tcx> { RejectedIsolatedOp(_) => ("operation rejected by isolation".to_string(), DiagLevel::Warning), Int2Ptr { .. } => ("integer-to-pointer cast".to_string(), DiagLevel::Warning), - NativeCallSharedMem => + NativeCallSharedMem { .. } => ("sharing memory with a native function".to_string(), DiagLevel::Warning), - NativeCallNoTrace => - ("unable to trace native code memory accesses".to_string(), DiagLevel::Warning), ExternTypeReborrow => ("reborrow of reference to `extern type`".to_string(), DiagLevel::Warning), CreatedPointerTag(..) @@ -666,11 +665,8 @@ impl<'tcx> MiriMachine<'tcx> { ProgressReport { .. } => format!("progress report: current operation being executed is here"), Int2Ptr { .. } => format!("integer-to-pointer cast"), - NativeCallSharedMem => format!("sharing memory with a native function called via FFI"), - NativeCallNoTrace => - format!( - "sharing memory with a native function called via FFI, and unable to use ptrace" - ), + NativeCallSharedMem { .. } => + format!("sharing memory with a native function called via FFI"), WeakMemoryOutdatedLoad { ptr } => format!("weak memory emulation: outdated value returned from load at {ptr}"), ExternTypeReborrow => @@ -716,42 +712,41 @@ impl<'tcx> MiriMachine<'tcx> { } v } - NativeCallSharedMem => { - vec![ - note!( - "when memory is shared with a native function call, Miri can only track initialisation and provenance on a best-effort basis" - ), - note!( - "in particular, Miri assumes that the native call initializes all memory it has written to" - ), - note!( - "Miri also assumes that any part of this memory may be a pointer that is permitted to point to arbitrary exposed memory" - ), - note!( - "what this means is that Miri will easily miss Undefined Behavior related to incorrect usage of this shared memory, so you should not take a clean Miri run as a signal that your FFI code is UB-free" - ), - ] - } - NativeCallNoTrace => { - vec![ - note!( - "when memory is shared with a native function call, Miri stops tracking initialization and provenance for that memory" - ), - note!( - "in particular, Miri assumes that the native call initializes all memory it has access to" - ), - note!( - "Miri also assumes that any part of this memory may be a pointer that is permitted to point to arbitrary exposed memory" - ), - note!( - "what this means is that Miri will easily miss Undefined Behavior related to incorrect usage of this shared memory, so you should not take a clean Miri run as a signal that your FFI code is UB-free" - ), - #[cfg(target_os = "linux")] - note!( - "this is normally partially mitigated, but either -Zmiri-force-old-native-lib-mode was passed or ptrace is disabled on your system" - ), - ] - } + NativeCallSharedMem { tracing } => + if *tracing { + vec![ + note!( + "when memory is shared with a native function call, Miri can only track initialisation and provenance on a best-effort basis" + ), + note!( + "in particular, Miri assumes that the native call initializes all memory it has written to" + ), + note!( + "Miri also assumes that any part of this memory may be a pointer that is permitted to point to arbitrary exposed memory" + ), + note!( + "what this means is that Miri will easily miss Undefined Behavior related to incorrect usage of this shared memory, so you should not take a clean Miri run as a signal that your FFI code is UB-free" + ), + note!( + "tracing memory accesses in native code is not yet fully implemented, so there can be further imprecisions beyond what is documented here" + ), + ] + } else { + vec![ + note!( + "when memory is shared with a native function call, Miri stops tracking initialization and provenance for that memory" + ), + note!( + "in particular, Miri assumes that the native call initializes all memory it has access to" + ), + note!( + "Miri also assumes that any part of this memory may be a pointer that is permitted to point to arbitrary exposed memory" + ), + note!( + "what this means is that Miri will easily miss Undefined Behavior related to incorrect usage of this shared memory, so you should not take a clean Miri run as a signal that your FFI code is UB-free" + ), + ] + }, ExternTypeReborrow => { assert!(self.borrow_tracker.as_ref().is_some_and(|b| { matches!( diff --git a/src/eval.rs b/src/eval.rs index c0dc484187..44612dae1e 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -150,8 +150,8 @@ pub struct MiriConfig { pub retag_fields: RetagFields, /// The location of the shared object files to load when calling external functions pub native_lib: Vec, - /// Whether to force using the old native lib behaviour even if ptrace might be supported. - pub force_old_native_lib: bool, + /// Whether to enable the new native lib tracing system. + pub native_lib_enable_tracing: bool, /// Run a garbage collector for BorTags every N basic blocks. pub gc_interval: u32, /// The number of CPUs to be reported by miri. @@ -201,7 +201,7 @@ impl Default for MiriConfig { report_progress: None, retag_fields: RetagFields::Yes, native_lib: vec![], - force_old_native_lib: false, + native_lib_enable_tracing: false, gc_interval: 10_000, num_cpus: 1, page_size: None, diff --git a/src/lib.rs b/src/lib.rs index b1c15b006f..850478b516 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -100,7 +100,9 @@ use rustc_middle::{bug, span_bug}; use tracing::{info, trace}; #[cfg(target_os = "linux")] -pub use crate::shims::trace::{init_sv, register_retcode_sv}; +pub mod native_lib { + pub use crate::shims::{init_sv, register_retcode_sv}; +} // Type aliases that set the provenance parameter. pub type Pointer = interpret::Pointer>; diff --git a/src/shims/mod.rs b/src/shims/mod.rs index e7b0a784c2..44e145dd5d 100644 --- a/src/shims/mod.rs +++ b/src/shims/mod.rs @@ -19,10 +19,10 @@ pub mod os_str; pub mod panic; pub mod time; pub mod tls; -#[cfg(target_os = "linux")] -pub mod trace; pub use self::files::FdTable; +#[cfg(target_os = "linux")] +pub use self::native_lib::trace::{init_sv, register_retcode_sv}; pub use self::unix::{DirTable, EpollInterestTable}; /// What needs to be done after emulating an item (a shim or an intrinsic) is done. diff --git a/src/shims/native_lib.rs b/src/shims/native_lib/mod.rs similarity index 64% rename from src/shims/native_lib.rs rename to src/shims/native_lib/mod.rs index 3e455e1738..f6f4766144 100644 --- a/src/shims/native_lib.rs +++ b/src/shims/native_lib/mod.rs @@ -1,7 +1,9 @@ //! Implements calling functions from a native library. -use std::ops::Deref; + #[cfg(target_os = "linux")] -use std::{cell::RefCell, rc::Rc}; +pub mod trace; + +use std::ops::Deref; use libffi::high::call as ffi; use libffi::low::CodePtr; @@ -11,12 +13,11 @@ use rustc_middle::ty::{self as ty, IntTy, UintTy}; use rustc_span::Symbol; #[cfg(target_os = "linux")] -use crate::alloc::isolated_alloc::IsolatedAlloc; +use self::trace::Supervisor; use crate::*; #[cfg(target_os = "linux")] -type CallResult<'tcx> = - InterpResult<'tcx, (ImmTy<'tcx>, Option)>; +type CallResult<'tcx> = InterpResult<'tcx, (ImmTy<'tcx>, Option)>; #[cfg(not(target_os = "linux"))] type CallResult<'tcx> = InterpResult<'tcx, (ImmTy<'tcx>, Option)>; @@ -32,84 +33,90 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> CallResult<'tcx> { let this = self.eval_context_mut(); #[cfg(target_os = "linux")] - let alloc = this.machine.allocator.clone(); - #[cfg(not(target_os = "linux"))] - let alloc = (); - let maybe_memevents; + let alloc = this.machine.allocator.as_ref().unwrap(); + + // SAFETY: We don't touch the machine memory past this point. + #[cfg(target_os = "linux")] + let (guard, stack_ptr) = unsafe { Supervisor::start_ffi(alloc) }; // Call the function (`ptr`) with arguments `libffi_args`, and obtain the return value // as the specified primitive integer type - let scalar = match dest.layout.ty.kind() { - // ints - ty::Int(IntTy::I8) => { - // Unsafe because of the call to native code. - // Because this is calling a C function it is not necessarily sound, - // but there is no way around this and we've checked as much as we can. - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_i8(x.0) - } - ty::Int(IntTy::I16) => { - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_i16(x.0) - } - ty::Int(IntTy::I32) => { - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_i32(x.0) - } - ty::Int(IntTy::I64) => { - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_i64(x.0) - } - ty::Int(IntTy::Isize) => { - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_target_isize(x.0.try_into().unwrap(), this) - } - // uints - ty::Uint(UintTy::U8) => { - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_u8(x.0) - } - ty::Uint(UintTy::U16) => { - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_u16(x.0) - } - ty::Uint(UintTy::U32) => { - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_u32(x.0) - } - ty::Uint(UintTy::U64) => { - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_u64(x.0) - } - ty::Uint(UintTy::Usize) => { - let x = unsafe { do_native_call::(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - Scalar::from_target_usize(x.0.try_into().unwrap(), this) - } - // Functions with no declared return type (i.e., the default return) - // have the output_type `Tuple([])`. - ty::Tuple(t_list) if (*t_list).deref().is_empty() => { - let (_, mm) = unsafe { do_native_call::<()>(ptr, libffi_args.as_slice(), alloc) }; - return interp_ok((ImmTy::uninit(dest.layout), mm)); - } - ty::RawPtr(..) => { - let x = unsafe { do_native_call::<*const ()>(ptr, libffi_args.as_slice(), alloc) }; - maybe_memevents = x.1; - let ptr = Pointer::new(Provenance::Wildcard, Size::from_bytes(x.0.addr())); - Scalar::from_pointer(ptr, this) - } - _ => throw_unsup_format!("unsupported return type for native call: {:?}", link_name), + let res = 'res: { + let scalar = match dest.layout.ty.kind() { + // ints + ty::Int(IntTy::I8) => { + // Unsafe because of the call to native code. + // Because this is calling a C function it is not necessarily sound, + // but there is no way around this and we've checked as much as we can. + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_i8(x) + } + ty::Int(IntTy::I16) => { + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_i16(x) + } + ty::Int(IntTy::I32) => { + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_i32(x) + } + ty::Int(IntTy::I64) => { + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_i64(x) + } + ty::Int(IntTy::Isize) => { + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_target_isize(x.try_into().unwrap(), this) + } + // uints + ty::Uint(UintTy::U8) => { + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_u8(x) + } + ty::Uint(UintTy::U16) => { + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_u16(x) + } + ty::Uint(UintTy::U32) => { + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_u32(x) + } + ty::Uint(UintTy::U64) => { + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_u64(x) + } + ty::Uint(UintTy::Usize) => { + let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + Scalar::from_target_usize(x.try_into().unwrap(), this) + } + // Functions with no declared return type (i.e., the default return) + // have the output_type `Tuple([])`. + ty::Tuple(t_list) if (*t_list).deref().is_empty() => { + unsafe { ffi::call::<()>(ptr, libffi_args.as_slice()) }; + break 'res interp_ok(ImmTy::uninit(dest.layout)); + } + ty::RawPtr(..) => { + let x = unsafe { ffi::call::<*const ()>(ptr, libffi_args.as_slice()) }; + let ptr = Pointer::new(Provenance::Wildcard, Size::from_bytes(x.addr())); + Scalar::from_pointer(ptr, this) + } + _ => + break 'res Err(err_unsup_format!( + "unsupported return type for native call: {:?}", + link_name + )) + .into(), + }; + interp_ok(ImmTy::from_scalar(scalar, dest.layout)) }; - interp_ok((ImmTy::from_scalar(scalar, dest.layout), maybe_memevents)) + + // SAFETY: We got the guard and stack pointer from start_ffi, and + // the allocator is the same + #[cfg(target_os = "linux")] + let events = unsafe { Supervisor::end_ffi(alloc, guard, stack_ptr) }; + #[cfg(not(target_os = "linux"))] + let events = None; + + interp_ok((res?, events)) } /// Get the pointer to the function of the specified name in the shared object file, @@ -205,14 +212,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // The first time this happens, print a warning. if !this.machine.native_call_mem_warned.replace(true) { // Newly set, so first time we get here. - #[cfg(target_os = "linux")] - if shims::trace::Supervisor::poll() { - this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem); - } else { - this.emit_diagnostic(NonHaltingDiagnostic::NativeCallNoTrace); - } - #[cfg(not(target_os = "linux"))] - this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem); + this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem { + #[cfg(target_os = "linux")] + tracing: self::trace::Supervisor::is_enabled(), + #[cfg(not(target_os = "linux"))] + tracing: false, + }); } this.expose_provenance(prov)?; @@ -243,48 +248,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } } -/// Performs the actual native call, returning the result and the events that -/// the supervisor detected (if any). -/// -/// SAFETY: See `libffi::fii::call`. -#[cfg(target_os = "linux")] -unsafe fn do_native_call( - ptr: CodePtr, - args: &[ffi::Arg<'_>], - alloc: Option>>, -) -> (T, Option) { - use shims::trace::Supervisor; - - unsafe { - if let Some(alloc) = alloc { - // SAFETY: We don't touch the machine memory past this point. - let (guard, stack_ptr) = Supervisor::start_ffi(alloc.clone()); - // SAFETY: Upheld by caller. - let ret = ffi::call(ptr, args); - // SAFETY: We got the guard and stack pointer from start_ffi, and - // the allocator is the same. - (ret, Supervisor::end_ffi(guard, alloc, stack_ptr)) - } else { - // SAFETY: Upheld by caller. - (ffi::call(ptr, args), None) - } - } -} - -/// Performs the actual native call, returning the result and a `None`. -/// Placeholder for platforms that do not support the ptrace supervisor. -/// -/// SAFETY: See `libffi::fii::call`. -#[cfg(not(target_os = "linux"))] -#[inline(always)] -unsafe fn do_native_call( - ptr: CodePtr, - args: &[ffi::Arg<'_>], - _alloc: (), -) -> (T, Option) { - (unsafe { ffi::call(ptr, args) }, None) -} - #[derive(Debug, Clone)] /// Enum of supported arguments to external C functions. // We introduce this enum instead of just calling `ffi::arg` and storing a list diff --git a/src/shims/trace/child.rs b/src/shims/native_lib/trace/child.rs similarity index 95% rename from src/shims/trace/child.rs rename to src/shims/native_lib/trace/child.rs index c320537b94..4961e875c7 100644 --- a/src/shims/trace/child.rs +++ b/src/shims/native_lib/trace/child.rs @@ -5,9 +5,9 @@ use ipc_channel::ipc; use nix::sys::{ptrace, signal}; use nix::unistd; -use super::messages::{Confirmation, MemEvents, TraceRequest}; +use super::CALLBACK_STACK_SIZE; +use super::messages::{Confirmation, MemEvents, StartFfiInfo, TraceRequest}; use super::parent::{ChildListener, sv_loop}; -use super::{FAKE_STACK_SIZE, StartFfiInfo}; use crate::alloc::isolated_alloc::IsolatedAlloc; static SUPERVISOR: std::sync::Mutex> = std::sync::Mutex::new(None); @@ -30,7 +30,7 @@ pub struct SvInitError; impl Supervisor { /// Returns `true` if the supervisor process exists, and `false` otherwise. - pub fn poll() -> bool { + pub fn is_enabled() -> bool { SUPERVISOR.lock().unwrap().is_some() } @@ -45,8 +45,8 @@ impl Supervisor { /// SAFETY: The resulting guard must be dropped *via `end_ffi`* immediately /// after the desired call has concluded. pub unsafe fn start_ffi( - alloc: Rc>, - ) -> (std::sync::MutexGuard<'static, Option>, Option<*mut [u8; FAKE_STACK_SIZE]>) + alloc: &Rc>, + ) -> (std::sync::MutexGuard<'static, Option>, Option<*mut [u8; CALLBACK_STACK_SIZE]>) { let mut sv_guard = SUPERVISOR.lock().unwrap(); // If the supervisor is not initialised for whatever reason, fast-fail. @@ -58,10 +58,10 @@ impl Supervisor { }; // Get pointers to all the pages the supervisor must allow accesses in - // and prepare the fake stack. + // and prepare the callback stack. let page_ptrs = alloc.borrow().pages(); - let raw_stack_ptr: *mut [u8; FAKE_STACK_SIZE] = - Box::leak(Box::new([0u8; FAKE_STACK_SIZE])).as_mut_ptr().cast(); + let raw_stack_ptr: *mut [u8; CALLBACK_STACK_SIZE] = + Box::leak(Box::new([0u8; CALLBACK_STACK_SIZE])).as_mut_ptr().cast(); let stack_ptr = raw_stack_ptr.expose_provenance(); let start_info = StartFfiInfo { page_ptrs, stack_ptr }; @@ -99,9 +99,9 @@ impl Supervisor { /// received by a prior call to `start_ffi`, and the allocator must be the /// one passed to it also. pub unsafe fn end_ffi( + alloc: &Rc>, mut sv_guard: std::sync::MutexGuard<'static, Option>, - alloc: Rc>, - raw_stack_ptr: Option<*mut [u8; FAKE_STACK_SIZE]>, + raw_stack_ptr: Option<*mut [u8; CALLBACK_STACK_SIZE]>, ) -> Option { // We can't use IPC channels here to signal that FFI mode has ended, // since they might allocate memory which could get us stuck in a SIGTRAP diff --git a/src/shims/trace/messages.rs b/src/shims/native_lib/trace/messages.rs similarity index 71% rename from src/shims/trace/messages.rs rename to src/shims/native_lib/trace/messages.rs index 1014ca750c..8a83dab5c0 100644 --- a/src/shims/trace/messages.rs +++ b/src/shims/native_lib/trace/messages.rs @@ -18,29 +18,42 @@ //! in `super::child` (namely `start_ffi()` and `end_ffi()`) to handle this. It is //! trivially easy to cause a deadlock or crash by messing this up! +use std::ops::Range; + /// An IPC request sent by the child process to the parent. /// /// The sender for this channel should live on the child process. #[derive(serde::Serialize, serde::Deserialize, Debug, Clone)] -pub(super) enum TraceRequest { +pub enum TraceRequest { /// Requests that tracing begins. Following this being sent, the child must /// wait to receive a `Confirmation` on the respective channel and then /// `raise(SIGSTOP)`. /// /// To avoid possible issues while allocating memory for IPC channels, ending /// the tracing is instead done via `raise(SIGUSR1)`. - StartFfi(super::StartFfiInfo), + StartFfi(StartFfiInfo), /// Manually overrides the code that the supervisor will return upon exiting. /// Once set, it is permanent. This can be called again to change the value. OverrideRetcode(i32), } +/// Information needed to begin tracing. +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone)] +pub struct StartFfiInfo { + /// A vector of page addresses. These should have been automatically obtained + /// with `IsolatedAlloc::pages` and prepared with `IsolatedAlloc::prepare_ffi`. + pub page_ptrs: Vec, + /// The address of an allocation that can serve as a temporary stack. + /// This should be a leaked `Box<[u8; CALLBACK_STACK_SIZE]>` cast to an int. + pub stack_ptr: usize, +} + /// A marker type confirming that the supervisor has received the request to begin /// tracing and is now waiting for a `SIGSTOP`. /// /// The sender for this channel should live on the parent process. #[derive(serde::Serialize, serde::Deserialize, Debug)] -pub(super) struct Confirmation; +pub struct Confirmation; /// The final results of an FFI trace, containing every relevant event detected /// by the tracer. Sent by the supervisor after receiving a `SIGUSR1` signal. @@ -53,5 +66,15 @@ pub struct MemEvents { /// pessimistically rounded up, and if the type (read/write/both) is uncertain /// it is reported as whatever would be safest to assume; i.e. a read + maybe-write /// becomes a read + write, etc. - pub acc_events: Vec, + pub acc_events: Vec, +} + +/// A single memory access, conservatively overestimated +/// in case of ambiguity. +#[derive(serde::Serialize, serde::Deserialize, Debug)] +pub enum AccessEvent { + /// A read may have occurred on no more than the specified address range. + Read(Range), + /// A write may have occurred on no more than the specified address range. + Write(Range), } diff --git a/src/shims/native_lib/trace/mod.rs b/src/shims/native_lib/trace/mod.rs new file mode 100644 index 0000000000..174b06b3ac --- /dev/null +++ b/src/shims/native_lib/trace/mod.rs @@ -0,0 +1,8 @@ +mod child; +pub mod messages; +mod parent; + +pub use self::child::{Supervisor, init_sv, register_retcode_sv}; + +/// The size of the temporary stack we use for callbacks that the server executes in the client. +const CALLBACK_STACK_SIZE: usize = 1024; diff --git a/src/shims/trace/parent.rs b/src/shims/native_lib/trace/parent.rs similarity index 84% rename from src/shims/trace/parent.rs rename to src/shims/native_lib/trace/parent.rs index cb9a9fc8da..1d968b7a97 100644 --- a/src/shims/trace/parent.rs +++ b/src/shims/native_lib/trace/parent.rs @@ -4,8 +4,8 @@ use ipc_channel::ipc; use nix::sys::{ptrace, signal, wait}; use nix::unistd; -use crate::shims::trace::messages::{Confirmation, MemEvents, TraceRequest}; -use crate::shims::trace::{AccessEvent, FAKE_STACK_SIZE, StartFfiInfo}; +use super::CALLBACK_STACK_SIZE; +use super::messages::{AccessEvent, Confirmation, MemEvents, StartFfiInfo, TraceRequest}; /// The flags to use when calling `waitid()`. /// Since bitwise or on the nix version of these flags is implemented as a trait, @@ -263,7 +263,7 @@ pub fn sv_loop( ExecEvent::Start(ch_info) => { // All the pages that the child process is "allowed to" access. ch_pages = ch_info.page_ptrs; - // And the fake stack it allocated for us to use later. + // And the temporary callback stack it allocated for us to use later. ch_stack = Some(ch_info.stack_ptr); // We received the signal and are no longer in the main listener loop, @@ -529,111 +529,113 @@ fn handle_segfault( let addr = unsafe { siginfo.si_addr().addr() }; let page_addr = addr.strict_sub(addr.strict_rem(page_size)); - if ch_pages.iter().any(|pg| (*pg..pg.strict_add(page_size)).contains(&addr)) { - // Overall structure: - // - Get the address that caused the segfault - // - Unprotect the memory - // - Step 1 instruction - // - Parse executed code to estimate size & type of access - // - Reprotect the memory - // - Continue - - // Ensure the stack is properly zeroed out! - for a in (ch_stack..ch_stack.strict_add(FAKE_STACK_SIZE)).step_by(ARCH_WORD_SIZE) { - ptrace::write(pid, std::ptr::with_exposed_provenance_mut(a), 0).unwrap(); - } - - // Guard against both architectures with upwards and downwards-growing stacks. - let stack_ptr = ch_stack.strict_add(FAKE_STACK_SIZE / 2); - let regs_bak = ptrace::getregs(pid).unwrap(); - let mut new_regs = regs_bak; - let ip_prestep = regs_bak.ip(); - - // Move the instr ptr into the deprotection code. - #[expect(clippy::as_conversions)] - new_regs.set_ip(mempr_off as usize); - // Don't mess up the stack by accident! - new_regs.set_sp(stack_ptr); - - // Modify the PAGE_ADDR global on the child process to point to the page - // that we want unprotected. - ptrace::write( - pid, - (&raw const PAGE_ADDR).cast_mut().cast(), - libc::c_long::try_from(page_addr).unwrap(), - ) - .unwrap(); - - // Check if we also own the next page, and if so unprotect it in case - // the access spans the page boundary. - let flag = if ch_pages.contains(&page_addr.strict_add(page_size)) { 2 } else { 1 }; - ptrace::write(pid, (&raw const PAGE_COUNT).cast_mut().cast(), flag).unwrap(); - - ptrace::setregs(pid, new_regs).unwrap(); - - // Our mempr_* functions end with a raise(SIGSTOP). - wait_for_signal(Some(pid), signal::SIGSTOP, true)?; - - // Step 1 instruction. - ptrace::setregs(pid, regs_bak).unwrap(); - ptrace::step(pid, None).unwrap(); - // Don't use wait_for_signal here since 1 instruction doesn't give room - // for any uncertainty + we don't want it `cont()`ing randomly by accident - // Also, don't let it continue with unprotected memory if something errors! - let _ = wait::waitid(wait::Id::Pid(pid), WAIT_FLAGS).map_err(|_| ExecEnd(None))?; - - // Zero out again to be safe - for a in (ch_stack..ch_stack.strict_add(FAKE_STACK_SIZE)).step_by(ARCH_WORD_SIZE) { - ptrace::write(pid, std::ptr::with_exposed_provenance_mut(a), 0).unwrap(); - } - - // Save registers and grab the bytes that were executed. This would - // be really nasty if it was a jump or similar but those thankfully - // won't do memory accesses and so can't trigger this! - let regs_bak = ptrace::getregs(pid).unwrap(); - new_regs = regs_bak; - let ip_poststep = regs_bak.ip(); - // We need to do reads/writes in word-sized chunks. - let diff = (ip_poststep.strict_sub(ip_prestep)).div_ceil(ARCH_WORD_SIZE); - let instr = (ip_prestep..ip_prestep.strict_add(diff)).fold(vec![], |mut ret, ip| { - // This only needs to be a valid pointer in the child process, not ours. - ret.append( - &mut ptrace::read(pid, std::ptr::without_provenance_mut(ip)) - .unwrap() - .to_ne_bytes() - .to_vec(), - ); - ret - }); - - // Now figure out the size + type of access and log it down - // This will mark down e.g. the same area being read multiple times, - // since it's more efficient to compress the accesses at the end. - if capstone_disassemble(&instr, addr, cs, acc_events).is_err() { - // Read goes first because we need to be pessimistic. - acc_events.push(AccessEvent::Read(addr..addr.strict_add(ARCH_MAX_ACCESS_SIZE))); - acc_events.push(AccessEvent::Write(addr..addr.strict_add(ARCH_MAX_ACCESS_SIZE))); - } - - // Reprotect everything and continue. - #[expect(clippy::as_conversions)] - new_regs.set_ip(mempr_on as usize); - new_regs.set_sp(stack_ptr); - ptrace::setregs(pid, new_regs).unwrap(); - wait_for_signal(Some(pid), signal::SIGSTOP, true)?; - - ptrace::setregs(pid, regs_bak).unwrap(); - ptrace::syscall(pid, None).unwrap(); - Ok(()) - } else { - // This was a real segfault, so print some debug info and quit. + if !ch_pages.iter().any(|pg| (*pg..pg.strict_add(page_size)).contains(&addr)) { + // This was a real segfault (not one of the Miri memory pages), so print some debug info and + // quit. let regs = ptrace::getregs(pid).unwrap(); eprintln!("Segfault occurred during FFI at {addr:#018x}"); eprintln!("Expected access on pages: {ch_pages:#018x?}"); eprintln!("Register dump: {regs:#x?}"); ptrace::kill(pid).unwrap(); - Err(ExecEnd(None)) + return Err(ExecEnd(None)); } + + // Overall structure: + // - Get the address that caused the segfault + // - Unprotect the memory: we force the child to execute `mempr_off`, passing parameters via + // global atomic variables. This is what we use the temporary callback stack for. + // - Step 1 instruction + // - Parse executed code to estimate size & type of access + // - Reprotect the memory by executing `mempr_on` in the child. + // - Continue + + // Ensure the stack is properly zeroed out! + for a in (ch_stack..ch_stack.strict_add(CALLBACK_STACK_SIZE)).step_by(ARCH_WORD_SIZE) { + ptrace::write(pid, std::ptr::with_exposed_provenance_mut(a), 0).unwrap(); + } + + // Guard against both architectures with upwards and downwards-growing stacks. + let stack_ptr = ch_stack.strict_add(CALLBACK_STACK_SIZE / 2); + let regs_bak = ptrace::getregs(pid).unwrap(); + let mut new_regs = regs_bak; + let ip_prestep = regs_bak.ip(); + + // Move the instr ptr into the deprotection code. + #[expect(clippy::as_conversions)] + new_regs.set_ip(mempr_off as usize); + // Don't mess up the stack by accident! + new_regs.set_sp(stack_ptr); + + // Modify the PAGE_ADDR global on the child process to point to the page + // that we want unprotected. + ptrace::write( + pid, + (&raw const PAGE_ADDR).cast_mut().cast(), + libc::c_long::try_from(page_addr).unwrap(), + ) + .unwrap(); + + // Check if we also own the next page, and if so unprotect it in case + // the access spans the page boundary. + let flag = if ch_pages.contains(&page_addr.strict_add(page_size)) { 2 } else { 1 }; + ptrace::write(pid, (&raw const PAGE_COUNT).cast_mut().cast(), flag).unwrap(); + + ptrace::setregs(pid, new_regs).unwrap(); + + // Our mempr_* functions end with a raise(SIGSTOP). + wait_for_signal(Some(pid), signal::SIGSTOP, true)?; + + // Step 1 instruction. + ptrace::setregs(pid, regs_bak).unwrap(); + ptrace::step(pid, None).unwrap(); + // Don't use wait_for_signal here since 1 instruction doesn't give room + // for any uncertainty + we don't want it `cont()`ing randomly by accident + // Also, don't let it continue with unprotected memory if something errors! + let _ = wait::waitid(wait::Id::Pid(pid), WAIT_FLAGS).map_err(|_| ExecEnd(None))?; + + // Zero out again to be safe + for a in (ch_stack..ch_stack.strict_add(CALLBACK_STACK_SIZE)).step_by(ARCH_WORD_SIZE) { + ptrace::write(pid, std::ptr::with_exposed_provenance_mut(a), 0).unwrap(); + } + + // Save registers and grab the bytes that were executed. This would + // be really nasty if it was a jump or similar but those thankfully + // won't do memory accesses and so can't trigger this! + let regs_bak = ptrace::getregs(pid).unwrap(); + new_regs = regs_bak; + let ip_poststep = regs_bak.ip(); + // We need to do reads/writes in word-sized chunks. + let diff = (ip_poststep.strict_sub(ip_prestep)).div_ceil(ARCH_WORD_SIZE); + let instr = (ip_prestep..ip_prestep.strict_add(diff)).fold(vec![], |mut ret, ip| { + // This only needs to be a valid pointer in the child process, not ours. + ret.append( + &mut ptrace::read(pid, std::ptr::without_provenance_mut(ip)) + .unwrap() + .to_ne_bytes() + .to_vec(), + ); + ret + }); + + // Now figure out the size + type of access and log it down. + // This will mark down e.g. the same area being read multiple times, + // since it's more efficient to compress the accesses at the end. + if capstone_disassemble(&instr, addr, cs, acc_events).is_err() { + // Read goes first because we need to be pessimistic. + acc_events.push(AccessEvent::Read(addr..addr.strict_add(ARCH_MAX_ACCESS_SIZE))); + acc_events.push(AccessEvent::Write(addr..addr.strict_add(ARCH_MAX_ACCESS_SIZE))); + } + + // Reprotect everything and continue. + #[expect(clippy::as_conversions)] + new_regs.set_ip(mempr_on as usize); + new_regs.set_sp(stack_ptr); + ptrace::setregs(pid, new_regs).unwrap(); + wait_for_signal(Some(pid), signal::SIGSTOP, true)?; + + ptrace::setregs(pid, regs_bak).unwrap(); + ptrace::syscall(pid, None).unwrap(); + Ok(()) } // We only get dropped into these functions via offsetting the instr pointer diff --git a/src/shims/trace/mod.rs b/src/shims/trace/mod.rs deleted file mode 100644 index a073482044..0000000000 --- a/src/shims/trace/mod.rs +++ /dev/null @@ -1,31 +0,0 @@ -mod child; -pub mod messages; -mod parent; - -use std::ops::Range; - -pub use self::child::{Supervisor, init_sv, register_retcode_sv}; - -/// The size used for the array into which we can move the stack pointer. -const FAKE_STACK_SIZE: usize = 1024; - -/// Information needed to begin tracing. -#[derive(serde::Serialize, serde::Deserialize, Debug, Clone)] -struct StartFfiInfo { - /// A vector of page addresses. These should have been automatically obtained - /// with `IsolatedAlloc::pages` and prepared with `IsolatedAlloc::prepare_ffi`. - page_ptrs: Vec, - /// The address of an allocation that can serve as a temporary stack. - /// This should be a leaked `Box<[u8; FAKE_STACK_SIZE]>` cast to an int. - stack_ptr: usize, -} - -/// A single memory access, conservatively overestimated -/// in case of ambiguity. -#[derive(serde::Serialize, serde::Deserialize, Debug)] -pub enum AccessEvent { - /// A read may have occurred on no more than the specified address range. - Read(Range), - /// A write may have occurred on no more than the specified address range. - Write(Range), -} diff --git a/tests/native-lib/pass/ptr_read_access.stderr b/tests/native-lib/pass/ptr_read_access.stderr index e256796e7a..04a3025bae 100644 --- a/tests/native-lib/pass/ptr_read_access.stderr +++ b/tests/native-lib/pass/ptr_read_access.stderr @@ -4,8 +4,8 @@ warning: sharing memory with a native function called via FFI LL | unsafe { print_pointer(&x) }; | ^^^^^^^^^^^^^^^^^ sharing memory with a native function | - = help: when memory is shared with a native function call, Miri can only track initialisation and provenance on a best-effort basis - = help: in particular, Miri assumes that the native call initializes all memory it has written to + = help: when memory is shared with a native function call, Miri stops tracking initialization and provenance for that memory + = help: in particular, Miri assumes that the native call initializes all memory it has access to = help: Miri also assumes that any part of this memory may be a pointer that is permitted to point to arbitrary exposed memory = help: what this means is that Miri will easily miss Undefined Behavior related to incorrect usage of this shared memory, so you should not take a clean Miri run as a signal that your FFI code is UB-free = note: BACKTRACE: diff --git a/tests/native-lib/pass/ptr_write_access.stderr b/tests/native-lib/pass/ptr_write_access.stderr index 53cbb05a32..c893b0d9f6 100644 --- a/tests/native-lib/pass/ptr_write_access.stderr +++ b/tests/native-lib/pass/ptr_write_access.stderr @@ -4,8 +4,8 @@ warning: sharing memory with a native function called via FFI LL | unsafe { increment_int(&mut x) }; | ^^^^^^^^^^^^^^^^^^^^^ sharing memory with a native function | - = help: when memory is shared with a native function call, Miri can only track initialisation and provenance on a best-effort basis - = help: in particular, Miri assumes that the native call initializes all memory it has written to + = help: when memory is shared with a native function call, Miri stops tracking initialization and provenance for that memory + = help: in particular, Miri assumes that the native call initializes all memory it has access to = help: Miri also assumes that any part of this memory may be a pointer that is permitted to point to arbitrary exposed memory = help: what this means is that Miri will easily miss Undefined Behavior related to incorrect usage of this shared memory, so you should not take a clean Miri run as a signal that your FFI code is UB-free = note: BACKTRACE: