From 2544feb33f5798cdd7c490ecf9bc7d3bb2175b7f Mon Sep 17 00:00:00 2001 From: jD91mZM2 Date: Sat, 3 Aug 2019 14:18:07 +0200 Subject: [PATCH 1/4] Disallow changing CS which contains CPL --- src/arch/x86_64/macros.rs | 4 +++- src/scheme/proc.rs | 28 ++++++++++++++-------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/arch/x86_64/macros.rs b/src/arch/x86_64/macros.rs index 5ad34d8..dbbd424 100644 --- a/src/arch/x86_64/macros.rs +++ b/src/arch/x86_64/macros.rs @@ -281,7 +281,9 @@ impl InterruptStack { self.scratch.rcx = all.rcx; self.scratch.rax = all.rax; self.iret.rip = all.rip; - self.iret.cs = all.cs; + + // These should probably be restricted + // self.iret.cs = all.cs; // self.iret.rflags = all.eflags; } /// Enables the "Trap Flag" in the FLAGS register, causing the CPU diff --git a/src/scheme/proc.rs b/src/scheme/proc.rs index b91831a..750ae64 100644 --- a/src/scheme/proc.rs +++ b/src/scheme/proc.rs @@ -26,20 +26,6 @@ use core::{ }; use spin::{Mutex, RwLock}; -#[derive(Clone, Copy)] -enum RegsKind { - Float, - Int -} -#[derive(Clone)] -enum Operation { - Memory(VirtualAddress), - Regs(RegsKind), - Trace { - clones: Vec - } -} - fn with_context(pid: ContextId, callback: F) -> Result where F: FnOnce(&Context) -> Result { @@ -101,6 +87,20 @@ fn try_stop_context(pid: ContextId, restart_after: bool, mut callback: F) } } +#[derive(Clone, Copy)] +enum RegsKind { + Float, + Int +} +#[derive(Clone)] +enum Operation { + Memory(VirtualAddress), + Regs(RegsKind), + Trace { + clones: Vec + } +} + #[derive(Clone, Copy)] struct Info { pid: ContextId, From 070f1fa913e5f5cbbda03e5cebe200274c68e728 Mon Sep 17 00:00:00 2001 From: jD91mZM2 Date: Sat, 3 Aug 2019 15:10:42 +0200 Subject: [PATCH 2/4] Delete ptrace-related TODO --- src/ptrace.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/ptrace.rs b/src/ptrace.rs index c296922..a720ea3 100644 --- a/src/ptrace.rs +++ b/src/ptrace.rs @@ -422,8 +422,12 @@ pub unsafe fn regs_for_mut(context: &mut Context) -> Option<&mut InterruptStack> pub fn with_context_memory(context: &Context, offset: VirtualAddress, len: usize, f: F) -> Result<()> where F: FnOnce(*mut u8) -> Result<()> { - // TODO: Is using USER_TMP_MISC_OFFSET safe? I guess make sure - // it's not too large. + // As far as I understand, mapping any regions following + // USER_TMP_MISC_OFFSET is safe because no other memory location + // is used after it. In the future it might be necessary to define + // a maximum amount of pages that can be mapped in one batch, + // which could be used to either internally retry `read`/`write` + // in `proc:/mem`, or return a partial read/write. let start = Page::containing_address(VirtualAddress::new(crate::USER_TMP_MISC_OFFSET)); let mut active_page_table = unsafe { ActivePageTable::new() }; From cf0a7620df02d5f0faacbe2dbbc83c51bb73f81b Mon Sep 17 00:00:00 2001 From: jD91mZM2 Date: Thu, 15 Aug 2019 14:23:54 +0200 Subject: [PATCH 3/4] Add ptrace exit breakpoint This will let you stop at process exit and inspect it right before the process dies. --- src/context/signal.rs | 1 - src/lib.rs | 1 - src/ptrace.rs | 8 +++++--- src/scheme/mod.rs | 6 +++--- src/scheme/sys/mod.rs | 2 +- src/syscall/process.rs | 9 ++++++--- syscall | 2 +- 7 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/context/signal.rs b/src/context/signal.rs index 942fb28..78e165a 100644 --- a/src/context/signal.rs +++ b/src/context/signal.rs @@ -1,6 +1,5 @@ use alloc::sync::Arc; use core::mem; -use syscall::data::PtraceEvent; use syscall::flag::{PTRACE_FLAG_IGNORE, PTRACE_STOP_SIGNAL, SIG_DFL, SIG_IGN, SIGCHLD, SIGCONT, SIGKILL, SIGSTOP, SIGTSTP, SIGTTIN, SIGTTOU}; use syscall::ptrace_event; diff --git a/src/lib.rs b/src/lib.rs index 24b265a..b7acc71 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,7 +13,6 @@ #![cfg_attr(feature = "clippy", allow(or_fun_call))] #![cfg_attr(feature = "clippy", allow(too_many_arguments))] #![deny(unreachable_patterns)] -#![feature(alloc)] #![feature(allocator_api)] #![feature(asm)] #![feature(concat_idents)] diff --git a/src/ptrace.rs b/src/ptrace.rs index a720ea3..b363196 100644 --- a/src/ptrace.rs +++ b/src/ptrace.rs @@ -209,8 +209,8 @@ pub fn set_breakpoint(pid: ContextId, flags: PtraceFlags) { /// Wait for the tracee to stop. If an event occurs, it returns a copy /// of that. It will still be available for read using recv_event. /// -/// Note: Don't call while holding any locks, this will switch -/// contexts +/// Note: Don't call while holding any locks or allocated data, this +/// will switch contexts and may in fact just never terminate. pub fn wait(pid: ContextId) -> Result<()> { let tracer: Arc = { let sessions = sessions(); @@ -238,7 +238,9 @@ pub fn wait(pid: ContextId) -> Result<()> { } /// Notify the tracer and await green flag to continue. -/// Note: Don't call while holding any locks, this will switch contexts +/// +/// Note: Don't call while holding any locks or allocated data, this +/// will switch contexts and may in fact just never terminate. pub fn breakpoint_callback(match_flags: PtraceFlags, event: Option) -> Option { // Can't hold any locks when executing wait() let (tracee, flags) = { diff --git a/src/scheme/mod.rs b/src/scheme/mod.rs index 8d2e345..fbdaba6 100644 --- a/src/scheme/mod.rs +++ b/src/scheme/mod.rs @@ -173,7 +173,7 @@ impl SchemeList { Ok(to) } - pub fn iter(&self) -> ::alloc::collections::btree_map::Iter>> { + pub fn iter(&self) -> ::alloc::collections::btree_map::Iter>> { self.map.iter() } @@ -188,7 +188,7 @@ impl SchemeList { self.map.get(&id) } - pub fn get_name(&self, ns: SchemeNamespace, name: &[u8]) -> Option<(SchemeId, &Arc>)> { + pub fn get_name(&self, ns: SchemeNamespace, name: &[u8]) -> Option<(SchemeId, &Arc>)> { if let Some(names) = self.names.get(&ns) { if let Some(&id) = names.get(name) { return self.get(id).map(|scheme| (id, scheme)); @@ -199,7 +199,7 @@ impl SchemeList { /// Create a new scheme. pub fn insert(&mut self, ns: SchemeNamespace, name: Box<[u8]>, scheme_fn: F) -> Result - where F: Fn(SchemeId) -> Arc> + where F: Fn(SchemeId) -> Arc> { if let Some(names) = self.names.get(&ns) { if names.contains_key(&name) { diff --git a/src/scheme/sys/mod.rs b/src/scheme/sys/mod.rs index ee99511..d12729d 100644 --- a/src/scheme/sys/mod.rs +++ b/src/scheme/sys/mod.rs @@ -27,7 +27,7 @@ struct Handle { seek: usize } -type SysFn = Fn() -> Result> + Send + Sync; +type SysFn = dyn Fn() -> Result> + Send + Sync; /// System information scheme pub struct SysScheme { diff --git a/src/syscall/process.rs b/src/syscall/process.rs index 646db9a..45baf3b 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -21,12 +21,13 @@ use crate::paging::{ActivePageTable, InactivePageTable, Page, VirtualAddress, PA use crate::{ptrace, syscall}; use crate::scheme::FileHandle; use crate::start::usermode; -use crate::syscall::data::{PtraceEvent, SigAction, Stat}; +use crate::syscall::data::{SigAction, Stat}; use crate::syscall::error::*; use crate::syscall::flag::{CloneFlags, CLONE_VFORK, CLONE_VM, CLONE_FS, CLONE_FILES, CLONE_SIGHAND, CLONE_STACK, MapFlags, PROT_EXEC, PROT_READ, PROT_WRITE, PTRACE_EVENT_CLONE, - SigActionFlags, SIG_DFL, SIG_BLOCK, SIG_UNBLOCK, SIG_SETMASK, SIGCONT, SIGTERM, - WaitFlags, WCONTINUED, WNOHANG, WUNTRACED, wifcontinued, wifstopped}; + PTRACE_STOP_EXIT, SigActionFlags, SIG_DFL, SIG_BLOCK, SIG_UNBLOCK, SIG_SETMASK, + SIGCONT, SIGTERM, WaitFlags, WCONTINUED, WNOHANG, WUNTRACED, wifcontinued, + wifstopped}; use crate::syscall::ptrace_event; use crate::syscall::validate::{validate_slice, validate_slice_mut}; @@ -1064,6 +1065,8 @@ pub fn exit(status: usize) -> ! { Arc::clone(&context_lock) }; + ptrace::breakpoint_callback(PTRACE_STOP_EXIT, Some(ptrace_event!(PTRACE_STOP_EXIT, status))); + let mut close_files = Vec::new(); let pid = { let mut context = context_lock.write(); diff --git a/syscall b/syscall index 0e1e7d5..6ba71e7 160000 --- a/syscall +++ b/syscall @@ -1 +1 @@ -Subproject commit 0e1e7d5d36f8c6f045e30c37515d366350eb2122 +Subproject commit 6ba71e7e065d37309b7d5c4fc86835dd350f61b7 From ab1a12ad4a10e26efcad5e2f2528f2763b45d545 Mon Sep 17 00:00:00 2001 From: jD91mZM2 Date: Thu, 15 Aug 2019 16:07:38 +0200 Subject: [PATCH 4/4] Remove deadlock-prone mutex in proc.rs I believe this could cause a deadlock if a blocking I/O operation was interrupted by a signal or otherwise, and decided to exit and close all files. It's unlikely to happen, but it can happen nontheless. This removes the mutex, but it's difficult to keep the code tidy. Hopefully this is good enough. --- src/scheme/proc.rs | 179 ++++++++++++++++++++++++++++----------------- 1 file changed, 110 insertions(+), 69 deletions(-) diff --git a/src/scheme/proc.rs b/src/scheme/proc.rs index 750ae64..3b66635 100644 --- a/src/scheme/proc.rs +++ b/src/scheme/proc.rs @@ -15,7 +15,6 @@ use crate::{ use alloc::{ collections::BTreeMap, - sync::Arc, vec::Vec }; use core::{ @@ -24,7 +23,7 @@ use core::{ slice, sync::atomic::{AtomicUsize, Ordering}, }; -use spin::{Mutex, RwLock}; +use spin::RwLock; fn with_context(pid: ContextId, callback: F) -> Result where F: FnOnce(&Context) -> Result @@ -87,17 +86,53 @@ fn try_stop_context(pid: ContextId, restart_after: bool, mut callback: F) } } -#[derive(Clone, Copy)] +#[derive(Clone, Copy, PartialEq, Eq)] enum RegsKind { Float, Int } -#[derive(Clone)] +#[derive(Clone, Copy, PartialEq, Eq)] enum Operation { - Memory(VirtualAddress), + Memory, Regs(RegsKind), - Trace { - clones: Vec + Trace, +} +struct MemData { + offset: VirtualAddress, +} +impl Default for MemData { + fn default() -> Self { + Self { offset: VirtualAddress::new(0) } + } +} +#[derive(Default)] +struct TraceData { + clones: Vec, +} +enum OperationData { + Memory(MemData), + Trace(TraceData), + Other, +} +impl OperationData { + fn default_for(op: Operation) -> OperationData { + match op { + Operation::Memory => OperationData::Memory(MemData::default()), + Operation::Trace => OperationData::Trace(TraceData::default()), + _ => OperationData::Other, + } + } + fn trace_data(&mut self) -> Option<&mut TraceData> { + match self { + OperationData::Trace(data) => Some(data), + _ => None, + } + } + fn mem_data(&mut self) -> Option<&mut MemData> { + match self { + OperationData::Memory(data) => Some(data), + _ => None, + } } } @@ -105,19 +140,21 @@ enum Operation { struct Info { pid: ContextId, flags: usize, + + // Important: Operation must never change. Search for: + // + // "operations can't change" to see usages. + operation: Operation, } struct Handle { info: Info, - operation: Operation + data: OperationData, } impl Handle { fn continue_ignored_children(&mut self) -> Option<()> { - let clones = match self.operation { - Operation::Trace { ref mut clones } => clones, - _ => return None - }; + let data = self.data.trace_data()?; let contexts = context::contexts(); - for pid in clones.drain(..) { + for pid in data.clones.drain(..) { if ptrace::is_traced(pid) { continue; } @@ -134,7 +171,7 @@ pub static PROC_SCHEME_ID: AtomicSchemeId = ATOMIC_SCHEMEID_INIT; pub struct ProcScheme { next_id: AtomicUsize, - handles: RwLock>>> + handles: RwLock>, } impl ProcScheme { @@ -156,13 +193,12 @@ impl Scheme for ProcScheme { .and_then(|s| s.parse().ok()) .map(ContextId::from) .ok_or(Error::new(EINVAL))?; + let operation = match parts.next() { - Some("mem") => Operation::Memory(VirtualAddress::new(0)), + Some("mem") => Operation::Memory, Some("regs/float") => Operation::Regs(RegsKind::Float), Some("regs/int") => Operation::Regs(RegsKind::Int), - Some("trace") => Operation::Trace { - clones: Vec::new() - }, + Some("trace") => Operation::Trace, _ => return Err(Error::new(EINVAL)) }; @@ -215,13 +251,14 @@ impl Scheme for ProcScheme { } } - self.handles.write().insert(id, Arc::new(Mutex::new(Handle { + self.handles.write().insert(id, Handle { info: Info { flags, pid, + operation, }, - operation, - }))); + data: OperationData::default_for(operation), + }); Ok(id) } @@ -236,7 +273,6 @@ impl Scheme for ProcScheme { let info = { let handles = self.handles.read(); let handle = handles.get(&old_id).ok_or(Error::new(EBADF))?; - let handle = handle.lock(); handle.info }; @@ -254,45 +290,45 @@ impl Scheme for ProcScheme { } fn seek(&self, id: usize, pos: usize, whence: usize) -> Result { - let handles = self.handles.read(); - let handle = handles.get(&id).ok_or(Error::new(EBADF))?; - let mut handle = handle.lock(); + let mut handles = self.handles.write(); + let handle = handles.get_mut(&id).ok_or(Error::new(EBADF))?; + let mut memory = handle.data.mem_data().ok_or(Error::new(EBADF))?; - match handle.operation { - Operation::Memory(ref mut offset) => Ok({ - *offset = VirtualAddress::new(match whence { - SEEK_SET => pos, - SEEK_CUR => cmp::max(0, offset.get() as isize + pos as isize) as usize, - SEEK_END => cmp::max(0, isize::max_value() + pos as isize) as usize, - _ => return Err(Error::new(EBADF)) - }); - offset.get() - }), - _ => Err(Error::new(EBADF)) - } + let value = match whence { + SEEK_SET => pos, + SEEK_CUR => cmp::max(0, memory.offset.get() as isize + pos as isize) as usize, + SEEK_END => cmp::max(0, isize::max_value() + pos as isize) as usize, + _ => return Err(Error::new(EBADF)) + }; + memory.offset = VirtualAddress::new(value); + Ok(value) } fn read(&self, id: usize, buf: &mut [u8]) -> Result { // Don't hold a global lock during the context switch later on - let handle = { + let info = { let handles = self.handles.read(); - Arc::clone(handles.get(&id).ok_or(Error::new(EBADF))?) + let handle = handles.get(&id).ok_or(Error::new(EBADF))?; + handle.info }; - let mut handle = handle.lock(); - let info = handle.info; - match handle.operation { - Operation::Memory(ref mut offset) => { + match info.operation { + Operation::Memory => { + // Won't context switch, don't worry about the locks + let mut handles = self.handles.write(); + let handle = handles.get_mut(&id).ok_or(Error::new(EBADF))?; + let data = handle.data.mem_data().expect("operations can't change"); + let contexts = context::contexts(); let context = contexts.get(info.pid).ok_or(Error::new(ESRCH))?; let context = context.read(); - ptrace::with_context_memory(&context, *offset, buf.len(), |ptr| { + ptrace::with_context_memory(&context, data.offset, buf.len(), |ptr| { buf.copy_from_slice(validate::validate_slice(ptr, buf.len())?); Ok(()) })?; - *offset = VirtualAddress::new(offset.get() + buf.len()); + data.offset = VirtualAddress::new(data.offset.get() + buf.len()); Ok(buf.len()) }, Operation::Regs(kind) => { @@ -332,7 +368,7 @@ impl Scheme for ProcScheme { Ok(len) }, - Operation::Trace { ref mut clones } => { + Operation::Trace => { let slice = unsafe { slice::from_raw_parts_mut( buf.as_mut_ptr() as *mut PtraceEvent, @@ -341,9 +377,14 @@ impl Scheme for ProcScheme { }; let read = ptrace::recv_events(info.pid, slice).unwrap_or(0); + // Won't context switch, don't worry about the locks + let mut handles = self.handles.write(); + let handle = handles.get_mut(&id).ok_or(Error::new(EBADF))?; + let data = handle.data.trace_data().expect("operations can't change"); + for event in &slice[..read] { if event.cause == PTRACE_EVENT_CLONE { - clones.push(ContextId::from(event.a)); + data.clones.push(ContextId::from(event.a)); } } @@ -354,26 +395,30 @@ impl Scheme for ProcScheme { fn write(&self, id: usize, buf: &[u8]) -> Result { // Don't hold a global lock during the context switch later on - let handle = { - let handles = self.handles.read(); - Arc::clone(handles.get(&id).ok_or(Error::new(EBADF))?) + let info = { + let mut handles = self.handles.write(); + let handle = handles.get_mut(&id).ok_or(Error::new(EBADF))?; + handle.continue_ignored_children(); + handle.info }; - let mut handle = handle.lock(); - let info = handle.info; - handle.continue_ignored_children(); - match handle.operation { - Operation::Memory(ref mut offset) => { + match info.operation { + Operation::Memory => { + // Won't context switch, don't worry about the locks + let mut handles = self.handles.write(); + let handle = handles.get_mut(&id).ok_or(Error::new(EBADF))?; + let data = handle.data.mem_data().expect("operations can't change"); + let contexts = context::contexts(); let context = contexts.get(info.pid).ok_or(Error::new(ESRCH))?; let context = context.read(); - ptrace::with_context_memory(&context, *offset, buf.len(), |ptr| { + ptrace::with_context_memory(&context, data.offset, buf.len(), |ptr| { validate::validate_slice_mut(ptr, buf.len())?.copy_from_slice(buf); Ok(()) })?; - *offset = VirtualAddress::new(offset.get() + buf.len()); + data.offset = VirtualAddress::new(data.offset.get() + buf.len()); Ok(buf.len()) }, Operation::Regs(kind) => match kind { @@ -416,7 +461,7 @@ impl Scheme for ProcScheme { }) } }, - Operation::Trace { .. } => { + Operation::Trace => { if buf.len() < mem::size_of::() { return Ok(0); } @@ -470,9 +515,8 @@ impl Scheme for ProcScheme { } fn fcntl(&self, id: usize, cmd: usize, arg: usize) -> Result { - let handles = self.handles.read(); - let handle = handles.get(&id).ok_or(Error::new(EBADF))?; - let mut handle = handle.lock(); + let mut handles = self.handles.write(); + let mut handle = handles.get_mut(&id).ok_or(Error::new(EBADF))?; match cmd { F_SETFL => { handle.info.flags = arg; Ok(0) }, @@ -484,7 +528,6 @@ impl Scheme for ProcScheme { fn fevent(&self, id: usize, _flags: EventFlags) -> Result { let handles = self.handles.read(); let handle = handles.get(&id).ok_or(Error::new(EBADF))?; - let handle = handle.lock(); Ok(ptrace::session_fevent_flags(handle.info.pid).expect("proc (fevent): invalid session")) } @@ -492,13 +535,12 @@ impl Scheme for ProcScheme { fn fpath(&self, id: usize, buf: &mut [u8]) -> Result { let handles = self.handles.read(); let handle = handles.get(&id).ok_or(Error::new(EBADF))?; - let handle = handle.lock(); - let path = format!("proc:{}/{}", handle.info.pid.into(), match handle.operation { - Operation::Memory(_) => "mem", + let path = format!("proc:{}/{}", handle.info.pid.into(), match handle.info.operation { + Operation::Memory => "mem", Operation::Regs(RegsKind::Float) => "regs/float", Operation::Regs(RegsKind::Int) => "regs/int", - Operation::Trace { .. } => "trace" + Operation::Trace => "trace" }); let len = cmp::min(path.len(), buf.len()); @@ -508,11 +550,10 @@ impl Scheme for ProcScheme { } fn close(&self, id: usize) -> Result { - let handle = self.handles.write().remove(&id).ok_or(Error::new(EBADF))?; - let mut handle = handle.lock(); + let mut handle = self.handles.write().remove(&id).ok_or(Error::new(EBADF))?; handle.continue_ignored_children(); - if let Operation::Trace { .. } = handle.operation { + if let Operation::Trace = handle.info.operation { ptrace::close_session(handle.info.pid); if handle.info.flags & O_EXCL == O_EXCL {