From 41883730206a10fa3043c87135418e829938ad65 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 14 Feb 2021 11:44:21 +0100 Subject: [PATCH] Kernel: Fix TOCTOU in syscall entry region validation We were doing stack and syscall-origin region validations before taking the big process lock. There was a window of time where those regions could then be unmapped/remapped by another thread before we proceed with our syscall. This patch closes that window, and makes sys$get_stack_bounds() rely on the fact that we now know the userspace stack pointer to be valid. Thanks to @BenWiederhake for spotting this! :^) --- Kernel/Syscall.cpp | 4 +++- Kernel/Syscalls/get_stack_bounds.cpp | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Kernel/Syscall.cpp b/Kernel/Syscall.cpp index 569a6434e9..f087d2a71f 100644 --- a/Kernel/Syscall.cpp +++ b/Kernel/Syscall.cpp @@ -169,6 +169,9 @@ void syscall_handler(TrapFrame* trap) PANIC("Syscall from process with IOPL != 0"); } + // NOTE: We take the big process lock before inspecting memory regions. + process.big_lock().lock(); + if (!MM.validate_user_stack(process, VirtualAddress(regs.userspace_esp))) { dbgln("Invalid stack pointer: {:p}", regs.userspace_esp); handle_crash(regs, "Bad stack on syscall entry", SIGSTKFLT); @@ -190,7 +193,6 @@ void syscall_handler(TrapFrame* trap) handle_crash(regs, "Syscall from non-syscall region", SIGSEGV); } - process.big_lock().lock(); u32 function = regs.eax; u32 arg1 = regs.edx; u32 arg2 = regs.ecx; diff --git a/Kernel/Syscalls/get_stack_bounds.cpp b/Kernel/Syscalls/get_stack_bounds.cpp index 0f764c736a..b72939220d 100644 --- a/Kernel/Syscalls/get_stack_bounds.cpp +++ b/Kernel/Syscalls/get_stack_bounds.cpp @@ -34,9 +34,9 @@ int Process::sys$get_stack_bounds(FlatPtr* user_stack_base, size_t* user_stack_s { FlatPtr stack_pointer = Thread::current()->get_register_dump_from_stack().userspace_esp; auto* stack_region = space().find_region_containing(Range { VirtualAddress(stack_pointer), 1 }); - if (!stack_region) { - PANIC("sys$get_stack_bounds: No stack region found for process"); - } + + // The syscall handler should have killed us if we had an invalid stack pointer. + ASSERT(stack_region); FlatPtr stack_base = stack_region->range().base().get(); size_t stack_size = stack_region->size();