From 4a3dc2dadd361c66227997a2a9729fc6ec7cc6fb Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Thu, 7 Mar 2024 20:01:44 +0100 Subject: [PATCH] Fix the SIGKILL fd leak bug. --- src/context/context.rs | 31 ++++++++++++++++++++----------- src/context/list.rs | 2 +- src/context/signal.rs | 16 ++++++++++++++-- src/context/switch.rs | 4 ++-- src/main.rs | 2 +- src/scheme/proc.rs | 2 +- src/scheme/user.rs | 25 +++++++++++++++++-------- src/syscall/mod.rs | 2 +- src/syscall/process.rs | 3 +-- 9 files changed, 58 insertions(+), 29 deletions(-) diff --git a/src/context/context.rs b/src/context/context.rs index 30bc7e509d..b9f468e45a 100644 --- a/src/context/context.rs +++ b/src/context/context.rs @@ -1,4 +1,5 @@ use alloc::{borrow::Cow, sync::Arc, vec::Vec}; +use syscall::{SIGKILL, SIGSTOP}; use core::{cmp::Ordering, mem, num::NonZeroUsize}; use spin::RwLock; @@ -23,7 +24,7 @@ use crate::syscall::{ /// Unique identifier for a context (i.e. `pid`). use ::core::sync::atomic::AtomicUsize; -use super::memory::{GrantFileRef, AddrSpaceWrapper}; +use super::{memory::{GrantFileRef, AddrSpaceWrapper}, empty_cr3}; int_like!(ContextId, AtomicContextId, usize, AtomicUsize); /// The status of a context - used for scheduling @@ -396,13 +397,14 @@ impl Context { } pub fn set_addr_space( &mut self, - addr_space: Arc, + addr_space: Option>, ) -> Option> { - if let Some(ref old) = self.addr_space && Arc::ptr_eq(old, &addr_space) { - return Some(addr_space); + if let (Some(ref old), Some(ref new)) = (&self.addr_space, &addr_space) && Arc::ptr_eq(old, new) { + return addr_space; }; if self.id == super::context_id() { + // TODO: Share more code with context::arch::switch_to. let this_percpu = PercpuBlock::current(); if let Some(ref prev_addrsp) = self.addr_space { @@ -410,19 +412,25 @@ impl Context { prev_addrsp.acquire_read().used_by.atomic_clear(this_percpu.cpu_id); } - *this_percpu.current_addrsp.borrow_mut() = Some(Arc::clone(&addr_space)); + let _old_addrsp = core::mem::replace(&mut *this_percpu.current_addrsp.borrow_mut(), addr_space.clone()); - let new_addrsp = addr_space.acquire_read(); - new_addrsp.used_by.atomic_set(this_percpu.cpu_id); + if let Some(ref new) = addr_space { + let new_addrsp = new.acquire_read(); + new_addrsp.used_by.atomic_set(this_percpu.cpu_id); - unsafe { - new_addrsp.table.utable.make_current(); + unsafe { + new_addrsp.table.utable.make_current(); + } + } else { + unsafe { + crate::paging::RmmA::set_table(rmm::TableKind::User, empty_cr3()); + } } } else { assert!(!self.running); } - self.addr_space.replace(addr_space) + core::mem::replace(&mut self.addr_space, addr_space) } pub fn empty_actions() -> Arc>> { Arc::new(RwLock::new(vec![( @@ -470,7 +478,8 @@ impl Context { impl SignalState { pub fn deliverable(&self) -> u64 { - self.pending & !self.procmask + const CANT_BLOCK: u64 = (1 << (SIGKILL - 1)) | (1 << (SIGSTOP - 1)); + self.pending & (CANT_BLOCK | !self.procmask) } } diff --git a/src/context/list.rs b/src/context/list.rs index 2e1be989e5..e0e6cb3375 100644 --- a/src/context/list.rs +++ b/src/context/list.rs @@ -113,7 +113,7 @@ impl ContextList { let context_lock = self.new_context()?; { let mut context = context_lock.write(); - let _ = context.set_addr_space(AddrSpaceWrapper::new()?); + let _ = context.set_addr_space(Some(AddrSpaceWrapper::new()?)); let mut stack_top = unsafe { stack.as_mut_ptr().add(KSTACK_SIZE) }; diff --git a/src/context/signal.rs b/src/context/signal.rs index d0e619502e..7aa604832b 100644 --- a/src/context/signal.rs +++ b/src/context/signal.rs @@ -42,7 +42,7 @@ pub fn kmain_signal_handler() { // TODO: Move everything but SIGKILL to userspace. SIGCONT and SIGSTOP do not necessarily need to // be done from this current context. -pub fn signal_handler(eintr: bool) { +pub fn signal_handler() { let (action, sig) = { // FIXME: Can any low-level state become corrupt if a panic occurs here? let context_lock = context::current().expect("context::signal_handler not inside of context"); @@ -53,7 +53,18 @@ pub fn signal_handler(eintr: bool) { if context.sig.deliverable() == 0 { return; } - let selected = context.sig.deliverable().trailing_zeros() as usize + 1; + let bits = context.sig.deliverable(); + + // Always prioritize SIGKILL and SIGSTOP, so processes can't simply keep themselves alive + // by killing themselves. + let selected = if bits & (1 << (SIGKILL - 1)) != 0 { + SIGKILL + } else if bits & (1 << (SIGSTOP - 1)) != 0 { + SIGSTOP + } else { + bits.trailing_zeros() as usize + 1 + }; + context.sig.pending &= !(1 << (selected - 1)); let actions = context.actions.read(); @@ -111,6 +122,7 @@ pub fn signal_handler(eintr: bool) { } } } + // FIXME: SIGSTOP shouldn't be overridable SIGSTOP | SIGTSTP | SIGTTIN | SIGTTOU => { // println!("Stop {}", sig); diff --git a/src/context/switch.rs b/src/context/switch.rs index 650b994747..d6924dbc4a 100644 --- a/src/context/switch.rs +++ b/src/context/switch.rs @@ -80,7 +80,7 @@ pub fn tick() { if new_ticks >= 3 { match switch() { SwitchResult::Switched { signal: true } => { - crate::context::signal::signal_handler(false); + crate::context::signal::signal_handler(); }, _ => (), } @@ -94,8 +94,8 @@ pub unsafe extern "C" fn switch_finish_hook() { // TODO: unreachable_unchecked()? crate::arch::stop::emergency_reset(); } - crate::percpu::switch_arch_hook(); arch::CONTEXT_SWITCH_LOCK.store(false, Ordering::SeqCst); + crate::percpu::switch_arch_hook(); } #[derive(Clone, Copy, Debug, Eq, PartialEq)] diff --git a/src/main.rs b/src/main.rs index b08b10f0fb..897473bd6f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -269,7 +269,7 @@ pub fn ksignal(signal: usize) { context.sig.pending |= 1 << (signal - 1); } } - crate::context::signal::signal_handler(false); + crate::context::signal::signal_handler(); } // TODO: Use this macro on aarch64 too. diff --git a/src/scheme/proc.rs b/src/scheme/proc.rs index 5f8b7c3c32..70de22263a 100644 --- a/src/scheme/proc.rs +++ b/src/scheme/proc.rs @@ -637,7 +637,7 @@ impl KernelScheme for ProcScheme { regs.set_instr_pointer(new_ip); regs.set_stack_pointer(new_sp); - Ok(context.set_addr_space(new)) + Ok(context.set_addr_space(Some(new))) })?; let _ = ptrace::send_event(crate::syscall::ptrace_event!( PTRACE_EVENT_ADDRSPACE_SWITCH, diff --git a/src/scheme/user.rs b/src/scheme/user.rs index f087176ba8..4c8bd585fe 100644 --- a/src/scheme/user.rs +++ b/src/scheme/user.rs @@ -15,7 +15,7 @@ use spin::{Mutex, RwLock}; use spinning_top::RwSpinlock; use syscall::{ FobtainFdFlags, MunmapFlags, SendFdFlags, MAP_FIXED_NOREPLACE, SKMSG_FOBTAINFD, - SKMSG_FRETURNFD, SKMSG_PROVIDE_MMAP, KSMSG_CANCEL, + SKMSG_FRETURNFD, SKMSG_PROVIDE_MMAP, KSMSG_CANCEL, SIGKILL, }; use crate::{ @@ -190,6 +190,14 @@ impl UserInner { loop { context::switch(); + let eintr_if_sigkill = || if context::current()?.read().sig.deliverable() & (1 << (SIGKILL - 1)) != 0 { + // EINTR directly if SIGKILL was found without waiting for scheme. Data loss + // doesn't matter. + Err(Error::new(EINTR)) + } else { + Ok(()) + }; + let mut states = self.states.lock(); match states.entry(id) { // invalid state @@ -198,12 +206,17 @@ impl UserInner { // signal wakeup while awaiting cancelation old_state @ State::Waiting { canceling: true, .. } => { *o.get_mut() = old_state; - continue; + drop(states); + eintr_if_sigkill()?; + context::current()?.write().block("UserInner::call"); } // spurious wakeup State::Waiting { canceling: false, fd, context } => { *o.get_mut() = State::Waiting { canceling: true, fd, context }; + drop(states); + eintr_if_sigkill()?; + // TODO: Is this too dangerous when the states lock is held? self.todo.send(Packet { id: 0, @@ -213,7 +226,7 @@ impl UserInner { ..packet }); event::trigger(self.root_id, self.handle_id, EVENT_READ); - continue; + context::current()?.write().block("UserInner::call"); } // invalid state @@ -585,10 +598,6 @@ impl UserInner { Ok(()) } fn handle_packet(&self, packet: &Packet) -> Result<()> { - if packet.a == KSMSG_CANCEL { - log::warn!("Context {} did KSMSG_CANCEL", context::current().unwrap().read().name); - } - if packet.id == 0 { // TODO: Simplify logic by using SKMSG with packet.id being ignored? match packet.a { @@ -597,7 +606,7 @@ impl UserInner { packet.b, EventFlags::from_bits_truncate(packet.c), ), - _ => log::warn!("Unknown scheme -> kernel message {}", packet.a), + _ => log::warn!("Unknown scheme -> kernel message {} from {}", packet.a, context::current().unwrap().read().name), } } else if Error::demux(packet.a) == Err(Error::new(ESKMSG)) { // The reason why the new ESKMSG mechanism was introduced, is that passing packet IDs diff --git a/src/syscall/mod.rs b/src/syscall/mod.rs index 1023374220..754278bf7c 100644 --- a/src/syscall/mod.rs +++ b/src/syscall/mod.rs @@ -328,7 +328,7 @@ pub fn syscall( // back to any given context, where the signal set/queue is nonempty, syscalls need to // complete *before* any signal is delivered. Otherwise the return value would probably be // overwritten. - crate::context::signal::signal_handler(true); + crate::context::signal::signal_handler(); } } } diff --git a/src/syscall/process.rs b/src/syscall/process.rs index e16620ecc7..3ac5346d32 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -43,8 +43,7 @@ pub fn exit(status: usize) -> ! { let mut context = context_lock.write(); close_files = Arc::try_unwrap(mem::take(&mut context.files)) .map_or_else(|_| Vec::new(), RwLock::into_inner); - addrspace_opt = - mem::take(&mut context.addr_space).and_then(|a| Arc::try_unwrap(a).ok()); + addrspace_opt = context.set_addr_space(None).and_then(|a| Arc::try_unwrap(a).ok()); drop(context.syscall_head.take()); drop(context.syscall_tail.take()); context.id