From 501952852ce84606618b82b703dedd505a11357a Mon Sep 17 00:00:00 2001
From: Ben Wiederhake <BenWiederhake.GitHub@gmx.de>
Date: Sun, 7 Mar 2021 13:21:03 +0100
Subject: [PATCH] Kernel: Fix pointer over/underflow in create_thread
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The expression
    (u8*)params.m_stack_location + stack_size
… causes UBSan to spit out the warning
    KUBSAN: addition of unsigned offset to 0x00000002 overflowed to 0xb0000003
… even though there is no actual overflow happening here.
This can be reproduced by running:
    $ syscall create_thread 0 [ 0 0 0 0 0xb0000001 2 ]
Technically, this is a true-positive: The C++-reference is incredibly strict
about pointer-arithmetic:
    > A pointer to non-array object is treated as a pointer to the first element
    > of an array with size 1. […] [A]ttempts to generate a pointer that isn't
    > pointing at an element of the same array or one past the end invoke
    > undefined behavior.
    https://en.cppreference.com/w/cpp/language/operator_arithmetic
Frankly, this feels silly. So let's just use FlatPtr instead.

Found by fuzz-syscalls. Undocumented bug.

Note that FlatPtr is an unsigned type, so
    user_esp.value() - 4
is defined even if we end up with a user_esp of 0 (this can happen for example
when params.m_stack_size = 0 and params.m_stack_location = 0). The result would
be a Kernelspace-pointer, which would then be immediately flagged by
'MM.validate_user_stack' as invalid, as intended.
---
 Kernel/Syscalls/thread.cpp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Kernel/Syscalls/thread.cpp b/Kernel/Syscalls/thread.cpp
index edefb7702e..3c00acc880 100644
--- a/Kernel/Syscalls/thread.cpp
+++ b/Kernel/Syscalls/thread.cpp
@@ -46,12 +46,12 @@ KResultOr<int> Process::sys$create_thread(void* (*entry)(void*), Userspace<const
     int schedule_priority = params.m_schedule_priority;
     unsigned stack_size = params.m_stack_size;
 
-    if (Checked<FlatPtr>::addition_would_overflow((FlatPtr)params.m_stack_location, stack_size))
+    auto user_esp = Checked<FlatPtr>((FlatPtr)params.m_stack_location);
+    user_esp += stack_size;
+    if (user_esp.has_overflow())
         return EOVERFLOW;
 
-    auto user_stack_address = (u8*)params.m_stack_location + stack_size;
-
-    if (!MM.validate_user_stack(*this, VirtualAddress(user_stack_address - 4)))
+    if (!MM.validate_user_stack(*this, VirtualAddress(user_esp.value() - 4)))
         return EFAULT;
 
     // FIXME: return EAGAIN if Thread::all_threads().size() is greater than PTHREAD_THREADS_MAX
@@ -83,7 +83,7 @@ KResultOr<int> Process::sys$create_thread(void* (*entry)(void*), Userspace<const
     tss.eip = (FlatPtr)entry;
     tss.eflags = 0x0202;
     tss.cr3 = space().page_directory().cr3();
-    tss.esp = (FlatPtr)user_stack_address;
+    tss.esp = user_esp.value();
 
     auto tsr_result = thread->make_thread_specific_region({});
     if (tsr_result.is_error())