From 485f51690d528379ef30a6a2e5c3d853cbc0f6c0 Mon Sep 17 00:00:00 2001 From: Brian Gianforcaro Date: Tue, 24 Aug 2021 12:53:47 -0700 Subject: [PATCH] Kernel: Always observe the return value of Region::map and remap We have seen cases where the map fails, but we return the region to the caller, causing them to page fault later on when they touch the region. The fix is to always observe the return code of map/remap. --- Kernel/Memory/AddressSpace.cpp | 10 ++++++++-- Kernel/Memory/MemoryManager.cpp | 3 ++- Kernel/Memory/Region.cpp | 10 +++++++--- Kernel/Syscalls/fork.cpp | 3 ++- Kernel/Syscalls/mmap.cpp | 14 +++++++++----- 5 files changed, 28 insertions(+), 12 deletions(-) diff --git a/Kernel/Memory/AddressSpace.cpp b/Kernel/Memory/AddressSpace.cpp index dabf79d3467..7f577a78d10 100644 --- a/Kernel/Memory/AddressSpace.cpp +++ b/Kernel/Memory/AddressSpace.cpp @@ -80,7 +80,10 @@ KResult AddressSpace::unmap_mmap_range(VirtualAddress addr, size_t size) // And finally we map the new region(s) using our page directory (they were just allocated and don't have one). for (auto* new_region : new_regions) { - new_region->map(page_directory()); + // TODO: Ideally we should do this in a way that can be rolled back on failure, as failing here + // leaves the caller in an undefined state. + if (!new_region->map(page_directory())) + return ENOMEM; } PerformanceManager::add_unmap_perf_event(Process::current(), range_to_unmap); @@ -130,7 +133,10 @@ KResult AddressSpace::unmap_mmap_range(VirtualAddress addr, size_t size) // And finally map the new region(s) into our page directory. for (auto* new_region : new_regions) { - new_region->map(page_directory()); + // TODO: Ideally we should do this in a way that can be rolled back on failure, as failing here + // leaves the caller in an undefined state. + if (!new_region->map(page_directory())) + return ENOMEM; } PerformanceManager::add_unmap_perf_event(Process::current(), range_to_unmap); diff --git a/Kernel/Memory/MemoryManager.cpp b/Kernel/Memory/MemoryManager.cpp index 324f9cb0836..9e5ad391753 100644 --- a/Kernel/Memory/MemoryManager.cpp +++ b/Kernel/Memory/MemoryManager.cpp @@ -748,7 +748,8 @@ OwnPtr MemoryManager::allocate_kernel_region_with_vmobject(VirtualRange return {}; auto region = maybe_region.release_value(); - region->map(kernel_page_directory()); + if (!region->map(kernel_page_directory())) + return {}; return region; } diff --git a/Kernel/Memory/Region.cpp b/Kernel/Memory/Region.cpp index c232e1bb7d7..7130f1c5ff3 100644 --- a/Kernel/Memory/Region.cpp +++ b/Kernel/Memory/Region.cpp @@ -285,7 +285,8 @@ bool Region::map(PageDirectory& page_directory, ShouldFlushTLB should_flush_tlb) void Region::remap() { VERIFY(m_page_directory); - map(*m_page_directory); + auto result = map(*m_page_directory); + VERIFY(result); } PageFaultResponse Region::handle_fault(PageFault const& fault) @@ -310,7 +311,8 @@ PageFaultResponse Region::handle_fault(PageFault const& fault) auto page_index_in_vmobject = translate_to_vmobject_page(page_index_in_region); VERIFY(m_vmobject->is_anonymous()); page_slot = static_cast(*m_vmobject).allocate_committed_page({}); - remap_vmobject_page(page_index_in_vmobject); + if (!remap_vmobject_page(page_index_in_vmobject)) + return PageFaultResponse::OutOfMemory; return PageFaultResponse::Continue; } dbgln("BUG! Unexpected NP fault at {}", fault.vaddr()); @@ -458,7 +460,9 @@ PageFaultResponse Region::handle_inode_fault(size_t page_index_in_region) MM.unquickmap_page(); } - remap_vmobject_page(page_index_in_vmobject); + if (!remap_vmobject_page(page_index_in_vmobject)) + return PageFaultResponse::OutOfMemory; + return PageFaultResponse::Continue; } diff --git a/Kernel/Syscalls/fork.cpp b/Kernel/Syscalls/fork.cpp index df64f4f12e3..ee3c7ca2a6d 100644 --- a/Kernel/Syscalls/fork.cpp +++ b/Kernel/Syscalls/fork.cpp @@ -109,7 +109,8 @@ KResultOr Process::sys$fork(RegisterState& regs) // TODO: tear down new process? return ENOMEM; } - child_region->map(child->address_space().page_directory(), Memory::ShouldFlushTLB::No); + if (!child_region->map(child->address_space().page_directory(), Memory::ShouldFlushTLB::No)) + return ENOMEM; if (region == m_master_tls_region.unsafe_ptr()) child->m_master_tls_region = child_region; diff --git a/Kernel/Syscalls/mmap.cpp b/Kernel/Syscalls/mmap.cpp index 4ad3550bb2d..6fcd86d2ae3 100644 --- a/Kernel/Syscalls/mmap.cpp +++ b/Kernel/Syscalls/mmap.cpp @@ -347,7 +347,7 @@ KResultOr Process::sys$mprotect(Userspace addr, size_t size, int } // Remove the old region from our regions tree, since were going to add another region - // with the exact same start address, but dont deallocate it yet + // with the exact same start address, but do not deallocate it yet auto region = address_space().take_region(*old_region); // Unmap the old region here, specifying that we *don't* want the VM deallocated. @@ -371,9 +371,11 @@ KResultOr Process::sys$mprotect(Userspace addr, size_t size, int // Map the new regions using our page directory (they were just allocated and don't have one). for (auto* adjacent_region : adjacent_regions) { - adjacent_region->map(address_space().page_directory()); + if (!adjacent_region->map(address_space().page_directory())) + return ENOMEM; } - new_region.map(address_space().page_directory()); + if (!new_region.map(address_space().page_directory())) + return ENOMEM; return 0; } @@ -438,9 +440,11 @@ KResultOr Process::sys$mprotect(Userspace addr, size_t size, int // Map the new region using our page directory (they were just allocated and don't have one) if any. if (adjacent_regions.size()) - adjacent_regions[0]->map(address_space().page_directory()); + if (!adjacent_regions[0]->map(address_space().page_directory())) + return ENOMEM; - new_region.map(address_space().page_directory()); + if (!new_region.map(address_space().page_directory())) + return ENOMEM; } return 0;