Skip to content

Commit

Permalink
Kernel: Fix TOCTOU in syscall entry region validation
Browse files Browse the repository at this point in the history
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! :^)
  • Loading branch information
awesomekling committed Feb 14, 2021
1 parent 10b7f6b commit 4188373
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 4 deletions.
4 changes: 3 additions & 1 deletion Kernel/Syscall.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions Kernel/Syscalls/get_stack_bounds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 4188373

Please sign in to comment.