beenull/ladybird
1
0
Fork 0
mirror of https://github.com/LadybirdBrowser/ladybird.git synced 2024-09-30 08:41:15 +00:00
Code Issues Actions 5 Packages Projects Releases Wiki Activity

Kernel: Fix pointer over/underflow in create_thread

Browse source 
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.
This commit is contained in:
Ben Wiederhake 2021-03-07 13:21:03 +01:00 committed by Andreas Kling
parent b6472204e5
commit 501952852c
Notes: sideshowbarker 2024-07-18 21:38:35 +09:00 
Author: https://github.com/BenWiederhake
Commit: https://github.com/SerenityOS/serenity/commit/501952852ce
Pull-request: https://github.com/SerenityOS/serenity/pull/5677
1 changed files with 5 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
Kernel/Syscalls/thread.cpp
View file

@ -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())