summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAndreas Kling <kling@serenityos.org>2021-02-14 11:44:21 +0100
committerAndreas Kling <kling@serenityos.org>2021-02-14 11:47:14 +0100
commit41883730206a10fa3043c87135418e829938ad65 (patch)
tree47d7f55eb146184dcce6b41cd5a22d3e8843254c
parent10b7f6b77eaf4e162f578139b75b1e2302966f49 (diff)
downloadserenity-41883730206a10fa3043c87135418e829938ad65.zip
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! :^)
-rw-r--r--Kernel/Syscall.cpp4
-rw-r--r--Kernel/Syscalls/get_stack_bounds.cpp6
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();