From 27e1120dffd98e55f45a6a175dc65b2eab22270d Mon Sep 17 00:00:00 2001
From: Brian Gianforcaro <bgianf@serenityos.org>
Date: Sun, 18 Jul 2021 09:31:28 -0700
Subject: [PATCH] Kernel: Move syscall precondition validates to MM

Move these to MM to simplify the flow of the syscall handler.

While here, also make sure we hold the process space lock for
the duration of the validation to avoid potential issues where
another thread attempts to modify the process space during the
validation. This will allow us to move the validation out of the
big process lock scope in a future change.

Additionally utilize the new no_lock variants of functions to avoid
unnecessary recursive process space spinlock acquisitions.
---
 Kernel/Syscall.cpp          | 34 +------------------------
 Kernel/VM/MemoryManager.cpp | 49 +++++++++++++++++++++++++++++++++++--
 Kernel/VM/MemoryManager.h   |  2 ++
 3 files changed, 50 insertions(+), 35 deletions(-)

diff --git a/Kernel/Syscall.cpp b/Kernel/Syscall.cpp
index 4f37b67383..38a6fa77f7 100644
--- a/Kernel/Syscall.cpp
+++ b/Kernel/Syscall.cpp
@@ -199,39 +199,7 @@ NEVER_INLINE void syscall_handler(TrapFrame* trap)
     // NOTE: We take the big process lock before inspecting memory regions.
     process.big_lock().lock();
 
-    VirtualAddress userspace_sp;
-#if ARCH(I386)
-    userspace_sp = VirtualAddress { regs.userspace_esp };
-#else
-    userspace_sp = VirtualAddress { regs.userspace_rsp };
-#endif
-    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);
-    }
-
-    VirtualAddress ip;
-#if ARCH(I386)
-    ip = VirtualAddress { regs.eip };
-#else
-    ip = VirtualAddress { regs.rip };
-#endif
-
-    auto* calling_region = MM.find_user_region_from_vaddr(process.space(), ip);
-    if (!calling_region) {
-        dbgln("Syscall from {:p} which has no associated region", ip);
-        handle_crash(regs, "Syscall from unknown region", SIGSEGV);
-    }
-
-    if (calling_region->is_writable()) {
-        dbgln("Syscall from writable memory at {:p}", ip);
-        handle_crash(regs, "Syscall from writable memory", SIGSEGV);
-    }
-
-    if (process.space().enforces_syscall_regions() && !calling_region->is_syscall_region()) {
-        dbgln("Syscall from non-syscall region");
-        handle_crash(regs, "Syscall from non-syscall region", SIGSEGV);
-    }
+    MM.validate_syscall_preconditions(process.space(), regs);
 
     FlatPtr function;
     FlatPtr arg1;
diff --git a/Kernel/VM/MemoryManager.cpp b/Kernel/VM/MemoryManager.cpp
index 30f8a54fc7..46767ac923 100644
--- a/Kernel/VM/MemoryManager.cpp
+++ b/Kernel/VM/MemoryManager.cpp
@@ -624,10 +624,55 @@ Region* MemoryManager::kernel_region_from_vaddr(VirtualAddress vaddr)
     return nullptr;
 }
 
+Region* MemoryManager::find_user_region_from_vaddr_no_lock(Space& space, VirtualAddress vaddr)
+{
+    VERIFY(space.get_lock().own_lock());
+    return space.find_region_containing({ vaddr, 1 });
+}
+
 Region* MemoryManager::find_user_region_from_vaddr(Space& space, VirtualAddress vaddr)
 {
     ScopedSpinLock lock(space.get_lock());
-    return space.find_region_containing({ vaddr, 1 });
+    return find_user_region_from_vaddr_no_lock(space, vaddr);
+}
+
+void MemoryManager::validate_syscall_preconditions(Space& space, RegisterState& regs)
+{
+    // We take the space lock once here and then use the no_lock variants
+    // to avoid excessive spinlock recursion in this extemely common path.
+    ScopedSpinLock lock(space.get_lock());
+
+    auto unlock_and_handle_crash = [&lock, &regs](const char* description, int signal) {
+        lock.unlock();
+        handle_crash(regs, description, signal);
+    };
+
+    {
+        VirtualAddress userspace_sp = VirtualAddress { regs.userspace_sp() };
+        if (!MM.validate_user_stack_no_lock(space, userspace_sp)) {
+            dbgln("Invalid stack pointer: {:p}", userspace_sp);
+            unlock_and_handle_crash("Bad stack on syscall entry", SIGSTKFLT);
+        }
+    }
+
+    {
+        VirtualAddress ip = VirtualAddress { regs.ip() };
+        auto* calling_region = MM.find_user_region_from_vaddr_no_lock(space, ip);
+        if (!calling_region) {
+            dbgln("Syscall from {:p} which has no associated region", ip);
+            unlock_and_handle_crash("Syscall from unknown region", SIGSEGV);
+        }
+
+        if (calling_region->is_writable()) {
+            dbgln("Syscall from writable memory at {:p}", ip);
+            unlock_and_handle_crash("Syscall from writable memory", SIGSEGV);
+        }
+
+        if (space.enforces_syscall_regions() && !calling_region->is_syscall_region()) {
+            dbgln("Syscall from non-syscall region");
+            unlock_and_handle_crash("Syscall from non-syscall region", SIGSEGV);
+        }
+    }
 }
 
 Region* MemoryManager::find_region_from_vaddr(VirtualAddress vaddr)
@@ -1039,7 +1084,7 @@ bool MemoryManager::validate_user_stack_no_lock(Space& space, VirtualAddress vad
     if (!is_user_address(vaddr))
         return false;
 
-    auto* region = find_user_region_from_vaddr(space, vaddr);
+    auto* region = find_user_region_from_vaddr_no_lock(space, vaddr);
     return region && region->is_user() && region->is_stack();
 }
 
diff --git a/Kernel/VM/MemoryManager.h b/Kernel/VM/MemoryManager.h
index 2e3acd1aa6..dbb31faae5 100644
--- a/Kernel/VM/MemoryManager.h
+++ b/Kernel/VM/MemoryManager.h
@@ -189,6 +189,8 @@ public:
     }
 
     static Region* find_user_region_from_vaddr(Space&, VirtualAddress);
+    static Region* find_user_region_from_vaddr_no_lock(Space&, VirtualAddress);
+    static void validate_syscall_preconditions(Space&, RegisterState&);
 
     void dump_kernel_regions();