From 68b361bd21bcf7d44dd2fe8d40f4059041b08966 Mon Sep 17 00:00:00 2001 From: Luke Date: Wed, 16 Sep 2020 18:47:47 +0100 Subject: [PATCH] Kernel: Return ENOMEM in more places There are plenty of places in the kernel that aren't checking if they actually got their allocation. This fixes some of them, but definitely not all. Fixes #3390 Fixes #3391 Also, let's make find_one_free_page() return nullptr if it doesn't get a free index. This stops the kernel crashing when out of memory and allows memory purging to take place again. Fixes #3487 --- Kernel/Devices/BXVGADevice.cpp | 3 ++- Kernel/Devices/MBVGADevice.cpp | 3 ++- Kernel/Devices/SB16.cpp | 6 +++++- Kernel/Interrupts/APIC.cpp | 4 ++++ Kernel/Syscalls/execve.cpp | 9 +++++++-- Kernel/Syscalls/thread.cpp | 4 +++- Kernel/Thread.cpp | 10 +++++++--- Kernel/Thread.h | 4 ++-- Kernel/VM/PhysicalRegion.cpp | 3 ++- 9 files changed, 34 insertions(+), 12 deletions(-) diff --git a/Kernel/Devices/BXVGADevice.cpp b/Kernel/Devices/BXVGADevice.cpp index 85040ed7472..f6e8a749d8a 100644 --- a/Kernel/Devices/BXVGADevice.cpp +++ b/Kernel/Devices/BXVGADevice.cpp @@ -192,8 +192,9 @@ KResultOr BXVGADevice::mmap(Process& process, FileDescription&, Virtual 0, "BXVGA Framebuffer", prot); + if (!region) + return KResult(-ENOMEM); dbg() << "BXVGADevice: mmap with size " << region->size() << " at " << region->vaddr(); - ASSERT(region); return region; } diff --git a/Kernel/Devices/MBVGADevice.cpp b/Kernel/Devices/MBVGADevice.cpp index 84998f14c83..5b3bfdbe609 100644 --- a/Kernel/Devices/MBVGADevice.cpp +++ b/Kernel/Devices/MBVGADevice.cpp @@ -68,8 +68,9 @@ KResultOr MBVGADevice::mmap(Process& process, FileDescription&, Virtual 0, "MBVGA Framebuffer", prot); + if (!region) + return KResult(-ENOMEM); dbg() << "MBVGADevice: mmap with size " << region->size() << " at " << region->vaddr(); - ASSERT(region); return region; } diff --git a/Kernel/Devices/SB16.cpp b/Kernel/Devices/SB16.cpp index eb6d15b9936..aab811536d3 100644 --- a/Kernel/Devices/SB16.cpp +++ b/Kernel/Devices/SB16.cpp @@ -235,8 +235,12 @@ KResultOr SB16::write(FileDescription&, size_t, const UserOrKernelBuffer { if (!m_dma_region) { auto page = MM.allocate_supervisor_physical_page(); + if (!page) + return KResult(-ENOMEM); auto vmobject = AnonymousVMObject::create_with_physical_page(*page); m_dma_region = MM.allocate_kernel_region_with_vmobject(*vmobject, PAGE_SIZE, "SB16 DMA buffer", Region::Access::Write); + if (!m_dma_region) + return KResult(-ENOMEM); } #ifdef SB16_DEBUG @@ -245,7 +249,7 @@ KResultOr SB16::write(FileDescription&, size_t, const UserOrKernelBuffer ASSERT(length <= PAGE_SIZE); const int BLOCK_SIZE = 32 * 1024; if (length > BLOCK_SIZE) { - return -ENOSPC; + return KResult(-ENOSPC); } u8 mode = (u8)SampleFormat::Signed | (u8)SampleFormat::Stereo; diff --git a/Kernel/Interrupts/APIC.cpp b/Kernel/Interrupts/APIC.cpp index 23af86d11fd..6728015b60a 100644 --- a/Kernel/Interrupts/APIC.cpp +++ b/Kernel/Interrupts/APIC.cpp @@ -244,6 +244,10 @@ bool APIC::init_bsp() set_base(apic_base); m_apic_base = MM.allocate_kernel_region(apic_base.page_base(), PAGE_ROUND_UP(1), {}, Region::Access::Read | Region::Access::Write); + if (!m_apic_base) { + klog() << "APIC: Failed to allocate memory for APIC base"; + return false; + } auto rsdp = ACPI::StaticParsing::find_rsdp(); if (!rsdp.has_value()) { diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index 0e95a4c29e0..03d1597ebc4 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -267,7 +267,10 @@ int Process::do_exec(NonnullRefPtr main_program_description, Ve // NOTE: We create the new stack before disabling interrupts since it will zero-fault // and we don't want to deal with faults after this point. - u32 new_userspace_esp = new_main_thread->make_userspace_stack_for_main_thread(move(arguments), move(environment), move(auxv)); + auto make_stack_result = new_main_thread->make_userspace_stack_for_main_thread(move(arguments), move(environment), move(auxv)); + if (make_stack_result.is_error()) + return make_stack_result.error(); + u32 new_userspace_esp = make_stack_result.value(); if (wait_for_tracer_at_next_execve()) Thread::current()->send_urgent_signal_to_self(SIGSTOP); @@ -287,7 +290,9 @@ int Process::do_exec(NonnullRefPtr main_program_description, Ve // FIXME: PID/TID ISSUE m_pid = new_main_thread->tid().value(); - new_main_thread->make_thread_specific_region({}); + auto tsr_result = new_main_thread->make_thread_specific_region({}); + if (tsr_result.is_error()) + return tsr_result.error(); new_main_thread->reset_fpu_state(); auto& tss = new_main_thread->m_tss; diff --git a/Kernel/Syscalls/thread.cpp b/Kernel/Syscalls/thread.cpp index 1f13f90ce3c..ca7db7b4d4a 100644 --- a/Kernel/Syscalls/thread.cpp +++ b/Kernel/Syscalls/thread.cpp @@ -79,7 +79,9 @@ int Process::sys$create_thread(void* (*entry)(void*), Userspacemake_thread_specific_region({}); + auto tsr_result = thread->make_thread_specific_region({}); + if (tsr_result.is_error()) + return tsr_result.error(); thread->set_state(Thread::State::Runnable); return thread->tid().value(); } diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index 6e08de4ea96..5a042e5b3b4 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -671,10 +671,11 @@ RegisterState& Thread::get_register_dump_from_stack() return *(RegisterState*)(kernel_stack_top() - sizeof(RegisterState)); } -u32 Thread::make_userspace_stack_for_main_thread(Vector arguments, Vector environment, Vector auxiliary_values) +KResultOr Thread::make_userspace_stack_for_main_thread(Vector arguments, Vector environment, Vector auxiliary_values) { auto* region = m_process->allocate_region(VirtualAddress(), default_userspace_stack_size, "Stack (Main thread)", PROT_READ | PROT_WRITE, false); - ASSERT(region); + if (!region) + return KResult(-ENOMEM); region->set_stack(true); FlatPtr new_esp = region->vaddr().offset(default_userspace_stack_size).get(); @@ -925,11 +926,13 @@ Vector Thread::raw_backtrace(FlatPtr ebp, FlatPtr eip) const return backtrace; } -void Thread::make_thread_specific_region(Badge) +KResult Thread::make_thread_specific_region(Badge) { size_t thread_specific_region_alignment = max(process().m_master_tls_alignment, alignof(ThreadSpecificData)); m_thread_specific_region_size = align_up_to(process().m_master_tls_size, thread_specific_region_alignment) + sizeof(ThreadSpecificData); auto* region = process().allocate_region({}, m_thread_specific_region_size, "Thread-specific", PROT_READ | PROT_WRITE, true); + if (!region) + return KResult(-ENOMEM); SmapDisabler disabler; auto* thread_specific_data = (ThreadSpecificData*)region->vaddr().offset(align_up_to(process().m_master_tls_size, thread_specific_region_alignment)).as_ptr(); auto* thread_local_storage = (u8*)((u8*)thread_specific_data) - align_up_to(process().m_master_tls_size, process().m_master_tls_alignment); @@ -937,6 +940,7 @@ void Thread::make_thread_specific_region(Badge) thread_specific_data->self = thread_specific_data; if (process().m_master_tls_size) memcpy(thread_local_storage, process().m_master_tls_region->vaddr().as_ptr(), process().m_master_tls_size); + return KSuccess; } const LogStream& operator<<(const LogStream& stream, const Thread& value) diff --git a/Kernel/Thread.h b/Kernel/Thread.h index 5e766643d6f..e997c344bfc 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -433,9 +433,9 @@ public: void set_default_signal_dispositions(); bool push_value_on_stack(FlatPtr); - u32 make_userspace_stack_for_main_thread(Vector arguments, Vector environment, Vector); + KResultOr make_userspace_stack_for_main_thread(Vector arguments, Vector environment, Vector); - void make_thread_specific_region(Badge); + KResult make_thread_specific_region(Badge); unsigned syscall_count() const { return m_syscall_count; } void did_syscall() { ++m_syscall_count; } diff --git a/Kernel/VM/PhysicalRegion.cpp b/Kernel/VM/PhysicalRegion.cpp index 085d33a01a4..84f1e765b09 100644 --- a/Kernel/VM/PhysicalRegion.cpp +++ b/Kernel/VM/PhysicalRegion.cpp @@ -105,7 +105,8 @@ Optional PhysicalRegion::find_one_free_page() return {}; } auto free_index = m_bitmap.find_one_anywhere_unset(m_free_hint); - ASSERT(free_index.has_value()); + if (!free_index.has_value()) + return {}; auto page_index = free_index.value(); m_bitmap.set(page_index, true);