From e05237485c50c309a0f79c000418d34e5278eebe Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 6 Feb 2019 17:28:14 +0100 Subject: [PATCH] Kernel: Various stability improvements. - Don't cli() in Process::do_exec() unless current is execing. Eventually this should go away once the scheduler is less retarded in the face of interrupts. - Improved memory access validation for ring0 processes. We now look at the kernel ELF header to determine if an access is appropriate. :^) It's very hackish but also kinda neat. - Have Process::die() put the process into a new "Dying" state where it can still get scheduled but no signals will be dispatched. This way we can keep executing in die() but won't get our EIP hijacked by signal dispatch. The main problem here was that die() wanted to take various locks. --- Kernel/MemoryManager.cpp | 1 - Kernel/Process.cpp | 42 ++++++++++++++++++++++++++++++++-------- Kernel/Process.h | 8 +++++--- Kernel/Scheduler.cpp | 4 ++-- 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/Kernel/MemoryManager.cpp b/Kernel/MemoryManager.cpp index 8f36b5b2bf..85d4779293 100644 --- a/Kernel/MemoryManager.cpp +++ b/Kernel/MemoryManager.cpp @@ -885,7 +885,6 @@ size_t Region::amount_shared() const PageDirectory::~PageDirectory() { - ASSERT_INTERRUPTS_DISABLED(); #ifdef MM_DEBUG dbgprintf("MM: ~PageDirectory K%x\n", this); #endif diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 2bbfafce3d..128b10af29 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -19,6 +19,7 @@ #include "KSyms.h" #include #include "MasterPTY.h" +#include "elf.h" //#define DEBUG_IO //#define TASK_DEBUG @@ -394,7 +395,8 @@ int Process::do_exec(const String& path, Vector&& arguments, Vector= ksym_lowest_address && laddr.get() <= ksym_highest_address; + auto* kernel_elf_header = (Elf32_Ehdr*)0xf000; + auto* kernel_program_headers = (Elf32_Phdr*)(0xf000 + kernel_elf_header->e_phoff); + for (unsigned i = 0; i < kernel_elf_header->e_phnum; ++i) { + auto& segment = kernel_program_headers[i]; + if (laddr.get() < segment.p_vaddr || laddr.get() > (segment.p_vaddr + segment.p_memsz)) + continue; + if (is_write && !(kernel_program_headers[i].p_flags & PF_W)) + return false; + if (!is_write && !(kernel_program_headers[i].p_flags & PF_R)) + return false; + return true; + } + // Returning true in this case means "it's not inside the kernel binary. let the other checks deal with it." + return true; } bool Process::validate_read_from_kernel(LinearAddress laddr) const @@ -1635,7 +1649,7 @@ bool Process::validate_read_from_kernel(LinearAddress laddr) const // This code allows access outside of the known used address ranges to get caught. InterruptDisabler disabler; - if (is_inside_kernel_code(laddr)) + if (check_kernel_memory_access(laddr, false)) return true; if (is_kmalloc_address(laddr.as_ptr())) return true; @@ -1645,7 +1659,7 @@ bool Process::validate_read_from_kernel(LinearAddress laddr) const bool Process::validate_read(const void* address, size_t size) const { if (is_ring0()) { - if (is_inside_kernel_code(LinearAddress((dword)address))) + if (check_kernel_memory_access(LinearAddress((dword)address), false)) return true; if (is_kmalloc_address(address)) return true; @@ -1667,6 +1681,8 @@ bool Process::validate_write(void* address, size_t size) const if (is_ring0()) { if (is_kmalloc_address(address)) return true; + if (check_kernel_memory_access(LinearAddress((dword)address), true)) + return true; } ASSERT(size); if (!size) @@ -2108,15 +2124,25 @@ int Process::sys$chmod(const char* pathname, mode_t mode) void Process::die() { + // This is pretty hairy wrt interrupts. Once we set_state(Dead), we never get scheduled again. + // For this reason, we mark ourselves as Dying, which prevents the scheduler from dispatching signals in this process. + set_state(Dying); + InterruptFlagSaver saver; + + // STI so we can take locks. + sti(); destroy_all_windows(); - set_state(Dead); m_fds.clear(); m_tty = nullptr; - InterruptDisabler disabler; + // CLI for Process::from_pid(). This should go away eventually. + cli(); if (auto* parent_process = Process::from_pid(m_ppid)) { parent_process->send_signal(SIGCHLD, this); } + + // Good night. + set_state(Dead); } size_t Process::amount_virtual() const diff --git a/Kernel/Process.h b/Kernel/Process.h index 51ba1744c2..fa516f5700 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -67,6 +67,7 @@ public: Running, Skip1SchedulerPass, Skip0SchedulerPasses, + Dying, Dead, BeingInspected, BlockedSleep, @@ -125,7 +126,7 @@ public: template static void for_each(Callback); template static void for_each_in_pgrp(pid_t, Callback); template static void for_each_in_state(State, Callback); - template static void for_each_not_in_state(State, Callback); + template static void for_each_living(Callback); template void for_each_child(Callback); bool tick() { ++m_ticks; return --m_ticks_left; } @@ -442,6 +443,7 @@ static inline const char* to_string(Process::State state) case Process::Invalid: return "Invalid"; case Process::Runnable: return "Runnable"; case Process::Running: return "Running"; + case Process::Dying: return "Dying"; case Process::Dead: return "Dead"; case Process::Skip1SchedulerPass: return "Skip1"; case Process::Skip0SchedulerPasses: return "Skip0"; @@ -516,12 +518,12 @@ inline void Process::for_each_in_state(State state, Callback callback) } template -inline void Process::for_each_not_in_state(State state, Callback callback) +inline void Process::for_each_living(Callback callback) { ASSERT_INTERRUPTS_DISABLED(); for (auto* process = g_processes->head(); process;) { auto* next_process = process->next(); - if (process->state() != state) + if (process->state() != Process::Dead && process->state() != Process::Dying) callback(*process); process = next_process; } diff --git a/Kernel/Scheduler.cpp b/Kernel/Scheduler.cpp index d6aea973c4..20f3b2020f 100644 --- a/Kernel/Scheduler.cpp +++ b/Kernel/Scheduler.cpp @@ -132,7 +132,7 @@ bool Scheduler::pick_next() // Dispatch any pending signals. // FIXME: Do we really need this to be a separate pass over the process list? - Process::for_each_not_in_state(Process::Dead, [] (auto& process) { + Process::for_each_living([] (auto& process) { if (!process.has_unmasked_pending_signals()) return true; // We know how to interrupt blocked processes, but if they are just executing @@ -170,7 +170,7 @@ bool Scheduler::pick_next() g_processes->append(g_processes->remove_head()); auto* process = g_processes->head(); - if (process->state() == Process::Runnable || process->state() == Process::Running) { + if (process->state() == Process::Runnable || process->state() == Process::Running || process->state() == Process::Dying) { #ifdef SCHEDULER_DEBUG dbgprintf("switch to %s(%u) @ %w:%x\n", process->name().characters(), process->pid(), process->tss().cs, process->tss().eip); #endif