From cde10120828ca805af2a5b012a5829d3468d36fd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 28 Jun 2025 11:36:02 +0200 Subject: [PATCH] de-intend some code, and extend comments --- .../miri/src/shims/native_lib/trace/child.rs | 12 +- .../miri/src/shims/native_lib/trace/mod.rs | 6 +- .../miri/src/shims/native_lib/trace/parent.rs | 205 +++++++++--------- 3 files changed, 112 insertions(+), 111 deletions(-) diff --git a/src/tools/miri/src/shims/native_lib/trace/child.rs b/src/tools/miri/src/shims/native_lib/trace/child.rs index 370d9fa837c7..f4b0371f8d0a 100644 --- a/src/tools/miri/src/shims/native_lib/trace/child.rs +++ b/src/tools/miri/src/shims/native_lib/trace/child.rs @@ -7,7 +7,7 @@ use nix::unistd; use super::messages::{Confirmation, MemEvents, TraceRequest}; use super::parent::{ChildListener, sv_loop}; -use super::{FAKE_STACK_SIZE, StartFfiInfo}; +use super::{CALLBACK_STACK_SIZE, StartFfiInfo}; use crate::alloc::isolated_alloc::IsolatedAlloc; static SUPERVISOR: std::sync::Mutex> = std::sync::Mutex::new(None); @@ -46,7 +46,7 @@ impl Supervisor { /// after the desired call has concluded. pub unsafe fn start_ffi( alloc: &Rc>, - ) -> (std::sync::MutexGuard<'static, Option>, Option<*mut [u8; FAKE_STACK_SIZE]>) + ) -> (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 }; @@ -101,7 +101,7 @@ impl Supervisor { pub unsafe fn end_ffi( alloc: &Rc>, mut sv_guard: std::sync::MutexGuard<'static, Option>, - 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/tools/miri/src/shims/native_lib/trace/mod.rs b/src/tools/miri/src/shims/native_lib/trace/mod.rs index a073482044eb..8ff96151600d 100644 --- a/src/tools/miri/src/shims/native_lib/trace/mod.rs +++ b/src/tools/miri/src/shims/native_lib/trace/mod.rs @@ -6,8 +6,8 @@ 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; +/// The size of the temporary stack we use for callbacks that the server executes in the client. +const CALLBACK_STACK_SIZE: usize = 1024; /// Information needed to begin tracing. #[derive(serde::Serialize, serde::Deserialize, Debug, Clone)] @@ -16,7 +16,7 @@ struct StartFfiInfo { /// 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. + /// This should be a leaked `Box<[u8; CALLBACK_STACK_SIZE]>` cast to an int. stack_ptr: usize, } diff --git a/src/tools/miri/src/shims/native_lib/trace/parent.rs b/src/tools/miri/src/shims/native_lib/trace/parent.rs index 0fec14a12902..0a0415c6e108 100644 --- a/src/tools/miri/src/shims/native_lib/trace/parent.rs +++ b/src/tools/miri/src/shims/native_lib/trace/parent.rs @@ -5,7 +5,7 @@ use nix::sys::{ptrace, signal, wait}; use nix::unistd; use super::messages::{Confirmation, MemEvents, TraceRequest}; -use super::{AccessEvent, FAKE_STACK_SIZE, StartFfiInfo}; +use super::{AccessEvent, CALLBACK_STACK_SIZE, StartFfiInfo}; /// 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,112 +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: we force the child to execute `mempr_off`, passing - // parameters via global atomic variables. - // - 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(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