From 308396bca1e02a1cd0f5013ca0e2dd5a1aa6660a Mon Sep 17 00:00:00 2001 From: Brian Gianforcaro Date: Sun, 18 Jul 2021 08:53:37 -0700 Subject: [PATCH] Kernel: No lock validate_user_stack variant, switch to Space as argument The entire process is not needed, just require the user to pass in the Space. Also provide no_lock variant to use when you already have the VM/Space lock acquired, to avoid unnecessary recursive spinlock acquisitions. --- Kernel/Arch/x86/common/Interrupts.cpp | 2 +- Kernel/Syscall.cpp | 2 +- Kernel/Syscalls/thread.cpp | 2 +- Kernel/VM/MemoryManager.cpp | 14 +++++++++++--- Kernel/VM/MemoryManager.h | 3 ++- 5 files changed, 16 insertions(+), 7 deletions(-) diff --git a/Kernel/Arch/x86/common/Interrupts.cpp b/Kernel/Arch/x86/common/Interrupts.cpp index 012fd9e8eb..f4935578e2 100644 --- a/Kernel/Arch/x86/common/Interrupts.cpp +++ b/Kernel/Arch/x86/common/Interrupts.cpp @@ -311,7 +311,7 @@ void page_fault_handler(TrapFrame* trap) }; VirtualAddress userspace_sp = VirtualAddress { regs.userspace_sp() }; - if (!faulted_in_kernel && !MM.validate_user_stack(current_thread->process(), userspace_sp)) { + if (!faulted_in_kernel && !MM.validate_user_stack(current_thread->process().space(), userspace_sp)) { dbgln("Invalid stack pointer: {}", userspace_sp); handle_crash(regs, "Bad stack on page fault", SIGSTKFLT); } diff --git a/Kernel/Syscall.cpp b/Kernel/Syscall.cpp index 14a4a0f818..0374823953 100644 --- a/Kernel/Syscall.cpp +++ b/Kernel/Syscall.cpp @@ -200,7 +200,7 @@ NEVER_INLINE void syscall_handler(TrapFrame* trap) #else userspace_sp = VirtualAddress { regs.userspace_rsp }; #endif - if (!MM.validate_user_stack(process, userspace_sp)) { + if (!MM.validate_user_stack(process.space(), userspace_sp)) { dbgln("Invalid stack pointer: {:p}", userspace_sp); handle_crash(regs, "Bad stack on syscall entry", SIGSTKFLT); } diff --git a/Kernel/Syscalls/thread.cpp b/Kernel/Syscalls/thread.cpp index b543568749..8375912590 100644 --- a/Kernel/Syscalls/thread.cpp +++ b/Kernel/Syscalls/thread.cpp @@ -33,7 +33,7 @@ KResultOr Process::sys$create_thread(void* (*entry)(void*), Userspacespace(), VirtualAddress(user_sp.value() - 4))) return EFAULT; // FIXME: return EAGAIN if Thread::all_threads().size() is greater than PTHREAD_THREADS_MAX diff --git a/Kernel/VM/MemoryManager.cpp b/Kernel/VM/MemoryManager.cpp index 76607faf4e..30f8a54fc7 100644 --- a/Kernel/VM/MemoryManager.cpp +++ b/Kernel/VM/MemoryManager.cpp @@ -1032,15 +1032,23 @@ void MemoryManager::unquickmap_page() mm_data.m_quickmap_in_use.unlock(mm_data.m_quickmap_prev_flags); } -bool MemoryManager::validate_user_stack(Process const& process, VirtualAddress vaddr) const +bool MemoryManager::validate_user_stack_no_lock(Space& space, VirtualAddress vaddr) const { + VERIFY(space.get_lock().own_lock()); + if (!is_user_address(vaddr)) return false; - ScopedSpinLock lock(s_mm_lock); - auto* region = find_user_region_from_vaddr(const_cast(process).space(), vaddr); + + auto* region = find_user_region_from_vaddr(space, vaddr); return region && region->is_user() && region->is_stack(); } +bool MemoryManager::validate_user_stack(Space& space, VirtualAddress vaddr) const +{ + ScopedSpinLock lock(space.get_lock()); + return validate_user_stack_no_lock(space, vaddr); +} + void MemoryManager::register_vmobject(VMObject& vmobject) { ScopedSpinLock lock(s_mm_lock); diff --git a/Kernel/VM/MemoryManager.h b/Kernel/VM/MemoryManager.h index 50fc7aea09..2e3acd1aa6 100644 --- a/Kernel/VM/MemoryManager.h +++ b/Kernel/VM/MemoryManager.h @@ -134,7 +134,8 @@ public: static void enter_process_paging_scope(Process&); static void enter_space(Space&); - bool validate_user_stack(Process const&, VirtualAddress) const; + bool validate_user_stack_no_lock(Space&, VirtualAddress) const; + bool validate_user_stack(Space&, VirtualAddress) const; enum class ShouldZeroFill { No,