diff --git a/kernel/src/process/signal/mod.rs b/kernel/src/process/signal/mod.rs index 4a824a61c..47fb9801b 100644 --- a/kernel/src/process/signal/mod.rs +++ b/kernel/src/process/signal/mod.rs @@ -17,7 +17,7 @@ use core::{mem, sync::atomic::Ordering}; use align_ext::AlignExt; use c_types::{siginfo_t, ucontext_t}; -use constants::SIGKILL; +use constants::SIGSEGV; pub use events::{SigEvents, SigEventsFilter}; use ostd::{cpu::UserContext, user::UserContextApi}; pub use pause::{with_signal_blocked, Pause}; @@ -119,7 +119,9 @@ pub fn handle_pending_signal( signal.to_info(), ) { debug!("Failed to handle user signal: {:?}", e); - do_exit_group(TermStatus::Killed(SIGKILL)); + // If signal handling fails, the process should be terminated with SIGSEGV. + // Ref: + do_exit_group(TermStatus::Killed(SIGSEGV)); } } SigAction::Dfl => { @@ -164,17 +166,17 @@ pub fn handle_user_signal( debug!("handler_addr = 0x{:x}", handler_addr); debug!("flags = {:?}", flags); debug!("restorer_addr = 0x{:x}", restorer_addr); - // FIXME: How to respect flags? + if flags.contains_unsupported_flag() { warn!("Unsupported Signal flags: {:?}", flags); } if !flags.contains(SigActionFlags::SA_NODEFER) { - // add current signal to mask + // Add current signal to mask mask += sig_num; } - // block signals in sigmask when running signal handler + // Block signals in sigmask when running signal handler let old_mask = ctx.posix_thread.sig_mask().load(Ordering::Relaxed); ctx.posix_thread .sig_mask() @@ -184,7 +186,7 @@ pub fn handle_user_signal( let mut stack_pointer = if let Some(sp) = use_alternate_signal_stack(ctx.thread_local) { sp as u64 } else { - // just use user stack + // Just use user stack user_ctx.stack_pointer() as u64 }; @@ -193,12 +195,12 @@ pub fn handle_user_signal( let user_space = ctx.user_space(); - // 1. write siginfo_t + // 1. Write siginfo_t stack_pointer -= mem::size_of::() as u64; user_space.write_val(stack_pointer as _, &sig_info)?; let siginfo_addr = stack_pointer; - // 2. write ucontext_t. + // 2. Write ucontext_t. stack_pointer = alloc_aligned_in_user_stack(stack_pointer, mem::size_of::(), 16)?; let mut ucontext = ucontext_t { uc_sigmask: mask.into(), @@ -223,30 +225,20 @@ pub fn handle_user_signal( .sig_context() .set(Some(ucontext_addr as Vaddr)); - // 3. Set the address of the trampoline code. + // 3. Write the address of the restorer code. if flags.contains(SigActionFlags::SA_RESTORER) { - // If contains SA_RESTORER flag, trampoline code is provided by libc in restorer_addr. - // We just store restorer_addr on user stack to allow user code just to trampoline code. + // If the SA_RESTORER flag is present, the restorer code address is provided by the user. stack_pointer = write_u64_to_user_stack(stack_pointer, restorer_addr as u64)?; - trace!("After set restorer addr: user_rsp = 0x{:x}", stack_pointer); - } else { - // Otherwise we create a trampoline. - // FIXME: This may cause problems if we read old_context from rsp. - const TRAMPOLINE: &[u8] = &[ - 0xb8, 0x0f, 0x00, 0x00, 0x00, // mov eax, 15(syscall number of rt_sigreturn) - 0x0f, 0x05, // syscall (call rt_sigreturn) - 0x90, // nop (for alignment) - ]; - stack_pointer -= TRAMPOLINE.len() as u64; - let trampoline_rip = stack_pointer; - user_space.write_bytes(stack_pointer as Vaddr, &mut VmReader::from(TRAMPOLINE))?; - stack_pointer = write_u64_to_user_stack(stack_pointer, trampoline_rip)?; + trace!( + "After writing restorer addr: user_rsp = 0x{:x}", + stack_pointer + ); } // 4. Set correct register values user_ctx.set_instruction_pointer(handler_addr as _); user_ctx.set_stack_pointer(stack_pointer as usize); - // parameters of signal handler + // Parameters of signal handler if flags.contains(SigActionFlags::SA_SIGINFO) { user_ctx.set_arguments(sig_num, siginfo_addr as usize, ucontext_addr as usize); } else { diff --git a/kernel/src/process/signal/sig_disposition.rs b/kernel/src/process/signal/sig_disposition.rs index 1b6ec30f2..1a1007871 100644 --- a/kernel/src/process/signal/sig_disposition.rs +++ b/kernel/src/process/signal/sig_disposition.rs @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MPL-2.0 use super::{constants::*, sig_action::SigAction, sig_num::SigNum}; +use crate::{prelude::*, process::signal::sig_action::SigActionFlags}; #[derive(Copy, Clone)] pub struct SigDispositions { @@ -26,9 +27,10 @@ impl SigDispositions { self.map[idx] } - pub fn set(&mut self, num: SigNum, sa: SigAction) -> SigAction { + pub fn set(&mut self, num: SigNum, sa: SigAction) -> Result { + check_sigaction(&sa)?; let idx = Self::num_to_idx(num); - core::mem::replace(&mut self.map[idx], sa) + Ok(core::mem::replace(&mut self.map[idx], sa)) } pub fn set_default(&mut self, num: SigNum) { @@ -52,3 +54,36 @@ impl SigDispositions { (num.as_u8() - MIN_STD_SIG_NUM) as usize } } + +fn check_sigaction(sig_action: &SigAction) -> Result<()> { + // Here we only check if the SA_RESTORER flag is set and restorer_addr is not 0. + // Note: Linux checks the SA_RESTORER flag when initializing the signal stack, + // whereas we have moved this check forward to prevent this action from being set. + // This may result in some differences from the behavior of Linux. + + let SigAction::User { + flags, + restorer_addr, + .. + } = sig_action + else { + return Ok(()); + }; + + if flags.contains(SigActionFlags::SA_RESTORER) && *restorer_addr != 0 { + return Ok(()); + } + + cfg_if::cfg_if! { + if #[cfg(target_arch = "x86_64")] { + // On x86-64, `SA_RESTORER` is mandatory and cannot be omitted. + // Ref: + return_errno_with_message!(Errno::EINVAL, "x86-64 should always use SA_RESTORER"); + } else { + // FIXME: The support for user-provided signal handlers + // without `SA_RESTORER` is arch-dependent. + // Other archs may need to handle scenarios where `SA_RESTORER` is omitted. + return_errno_with_message!(Errno::EINVAL, "TODO: properly deal with SA_RESTORER"); + } + } +}