From 240d91f9514bf0b9190ba7b3f72729b5b1fd4508 Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Thu, 7 Jul 2022 12:11:58 +0200 Subject: [PATCH] Set address space/files when closing, not writing. This fixes file descriptor leaks. Suppose relibc is just about to set the address space. For this, it needs to write the address space fd to the selection fd. To avoid having to close them in the kernel, it rather memorizes what the file descriptors refer to internally, and then do the actual operation when they are gone, i.e. when closing. --- src/scheme/mod.rs | 5 --- src/scheme/proc.rs | 83 +++++++++++++++++++++++++++------------------- src/scheme/user.rs | 18 +++++----- 3 files changed, 56 insertions(+), 50 deletions(-) diff --git a/src/scheme/mod.rs b/src/scheme/mod.rs index 4e89661..6f66a44 100644 --- a/src/scheme/mod.rs +++ b/src/scheme/mod.rs @@ -302,11 +302,6 @@ pub fn schemes_mut() -> RwLockWriteGuard<'static, SchemeList> { #[allow(unused_variables)] pub trait KernelScheme: Scheme + Send + Sync + 'static { - fn kfmap(&self, number: usize, map: &syscall::data::Map, target_context: &Arc>) -> Result { - log::error!("Returning ENOSYS since kfmap can only be called on UserScheme schemes"); - Err(Error::new(ENOSYS)) - } - fn as_filetable(&self, number: usize) -> Result>>>> { Err(Error::new(EBADF)) } diff --git a/src/scheme/proc.rs b/src/scheme/proc.rs index b4117d4..05da3cf 100644 --- a/src/scheme/proc.rs +++ b/src/scheme/proc.rs @@ -120,7 +120,20 @@ enum Operation { Filetable { filetable: Arc>>> }, AddrSpace { addrspace: Arc> }, CurrentAddrSpace, + + // "operations CAN change". The reason we split changing the address space into two handle + // types, is that we would rather want the actual switch to occur when closing, as opposed to + // when writing. This is so that we can actually guarantee that no file descriptors are leaked. + AwaitingAddrSpaceChange { + new: Arc>, + new_sp: usize, + new_ip: usize, + }, + CurrentFiletable, + + AwaitingFiletableChange(Arc>>>), + // TODO: Remove this once openat is implemented, or allow openat-via-dup via e.g. the top-level // directory. OpenViaDup, @@ -530,8 +543,7 @@ impl Scheme for ProcScheme { // TODO: Define a struct somewhere? const RECORD_SIZE: usize = mem::size_of::() * 4; - let start = core::cmp::min(buf.len(), *offset); - let records = buf[start..].array_chunks_mut::(); + let records = buf.array_chunks_mut::(); let addrspace = addrspace.read(); let mut bytes_read = 0; @@ -682,8 +694,7 @@ impl Scheme for ProcScheme { // TODO: Find a better way to switch address spaces, since they also require switching // the instruction and stack pointer. Maybe remove `/regs` altogether and replace it // with `/ctx` - Operation::CurrentAddrSpace | Operation::CurrentFiletable => return Err(Error::new(EBADF)), - Operation::OpenViaDup | Operation::GrantHandle { .. } => return Err(Error::new(EBADF)), + _ => return Err(Error::new(EBADF)), } } @@ -996,12 +1007,8 @@ impl Scheme for ProcScheme { let mut filetable = hopefully_this_scheme.as_filetable(number)?; - let stopper = if info.pid == context::context_id() { with_context_mut } else { try_stop_context }; + self.handles.write().get_mut(&id).ok_or(Error::new(EBADF))?.info.operation = Operation::AwaitingFiletableChange(filetable); - stopper(info.pid, |context: &mut Context| { - context.files = filetable; - Ok(()) - })?; Ok(mem::size_of::()) } Operation::CurrentAddrSpace { .. } => { @@ -1013,25 +1020,11 @@ impl Scheme for ProcScheme { let (hopefully_this_scheme, number) = extract_scheme_number(addrspace_fd)?; let space = hopefully_this_scheme.as_addrspace(number)?; - let callback = |context: &mut Context| unsafe { - if let Some(saved_regs) = ptrace::regs_for_mut(context) { - saved_regs.iret.rip = ip; - saved_regs.iret.rsp = sp; - } else { - context.clone_entry = Some([ip, sp]); - } + self.handles.write().get_mut(&id).ok_or(Error::new(EBADF))?.info.operation = Operation::AwaitingAddrSpaceChange { new: space, new_sp: sp, new_ip: ip }; - context.set_addr_space(space); - Ok(()) - }; - if info.pid == context::context_id() { - with_context_mut(info.pid, callback)?; - } else { - try_stop_context(info.pid, callback)?; - } Ok(3 * mem::size_of::()) } - Operation::OpenViaDup | Operation::GrantHandle { .. } => return Err(Error::new(EBADF)), + _ => return Err(Error::new(EBADF)), } } @@ -1080,7 +1073,7 @@ impl Scheme for ProcScheme { Operation::CurrentFiletable => "current-filetable", Operation::OpenViaDup => "open-via-dup", - Operation::GrantHandle { .. } => return Err(Error::new(EOPNOTSUPP)), + _ => return Err(Error::new(EOPNOTSUPP)), }); read_from(buf, &path.as_bytes(), &mut 0) @@ -1111,18 +1104,38 @@ impl Scheme for ProcScheme { let mut handle = self.handles.write().remove(&id).ok_or(Error::new(EBADF))?; handle.continue_ignored_children(); - if let Operation::Trace = handle.info.operation { - ptrace::close_session(handle.info.pid); + let stop_context = if handle.info.pid == context::context_id() { with_context_mut } else { try_stop_context }; - if handle.info.flags & O_EXCL == O_EXCL { - syscall::kill(handle.info.pid, SIGKILL)?; - } + match handle.info.operation { + Operation::AwaitingAddrSpaceChange { new, new_sp, new_ip } => stop_context(handle.info.pid, |context: &mut Context| unsafe { + if let Some(saved_regs) = ptrace::regs_for_mut(context) { + saved_regs.iret.rip = new_ip; + saved_regs.iret.rsp = new_sp; + } else { + context.clone_entry = Some([new_ip, new_sp]); + } - let contexts = context::contexts(); - if let Some(context) = contexts.get(handle.info.pid) { - let mut context = context.write(); - context.ptrace_stop = false; + context.set_addr_space(new); + Ok(()) + })?, + Operation::AwaitingFiletableChange(new) => with_context_mut(handle.info.pid, |context: &mut Context| { + context.files = new; + Ok(()) + })?, + Operation::Trace => { + ptrace::close_session(handle.info.pid); + + if handle.info.flags & O_EXCL == O_EXCL { + syscall::kill(handle.info.pid, SIGKILL)?; + } + + let contexts = context::contexts(); + if let Some(context) = contexts.get(handle.info.pid) { + let mut context = context.write(); + context.ptrace_stop = false; + } } + _ => (), } Ok(0) } diff --git a/src/scheme/user.rs b/src/scheme/user.rs index 96101db..3fce59a 100644 --- a/src/scheme/user.rs +++ b/src/scheme/user.rs @@ -279,9 +279,13 @@ impl UserInner { Ok(0) } - fn fmap_inner(&self, file: usize, map: &Map, context_lock: &Arc>) -> Result { + fn fmap_inner(&self, file: usize, map: &Map) -> Result { let (pid, uid, gid, context_weak, desc) = { + let context_lock = Arc::clone(context::contexts().current().ok_or(Error::new(ESRCH))?); let context = context_lock.read(); + if map.size % PAGE_SIZE != 0 { + log::warn!("Unaligned map size for context {:?}", context.name.try_read().as_deref()); + } // TODO: Faster, cleaner mechanism to get descriptor let scheme = self.scheme_id.load(Ordering::SeqCst); let mut desc_res = Err(Error::new(EBADF)); @@ -298,9 +302,8 @@ impl UserInner { } } let desc = desc_res?; - (context.id, context.euid, context.egid, Arc::downgrade(context_lock), desc) + (context.id, context.euid, context.egid, Arc::downgrade(&context_lock), desc) }; - drop(context_lock); let address = self.capture(map)?; @@ -433,7 +436,7 @@ impl Scheme for UserScheme { fn fmap(&self, file: usize, map: &Map) -> Result { let inner = self.inner.upgrade().ok_or(Error::new(ENODEV))?; - inner.fmap_inner(file, map, &Arc::clone(context::contexts().current().ok_or(Error::new(ESRCH))?)) + inner.fmap_inner(file, map) } fn funmap(&self, grant_address: usize, size: usize) -> Result { @@ -535,9 +538,4 @@ impl Scheme for UserScheme { inner.call(SYS_CLOSE, file, 0, 0) } } -impl crate::scheme::KernelScheme for UserScheme { - fn kfmap(&self, number: usize, map: &Map, target_context: &Arc>) -> Result { - let inner = self.inner.upgrade().ok_or(Error::new(ENODEV))?; - inner.fmap_inner(number, map, target_context) - } -} +impl crate::scheme::KernelScheme for UserScheme {}