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.

5 files changed, 28 insertions, 12 deletions

diff --git a/Kernel/Memory/AddressSpace.cpp b/Kernel/Memory/AddressSpace.cpp
index dabf79d346..7f577a78d1 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 324f9cb083..9e5ad39175 100644
--- a/Kernel/Memory/MemoryManager.cpp
+++ b/Kernel/Memory/MemoryManager.cpp
@@ -748,7 +748,8 @@ OwnPtr<Region> 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 c232e1bb7d..7130f1c5ff 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<AnonymousVMObject&>(*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 df64f4f12e..ee3c7ca2a6 100644
--- a/Kernel/Syscalls/fork.cpp
+++ b/Kernel/Syscalls/fork.cpp
@@ -109,7 +109,8 @@ KResultOr<FlatPtr> 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 4ad3550bb2..6fcd86d2ae 100644
--- a/Kernel/Syscalls/mmap.cpp
+++ b/Kernel/Syscalls/mmap.cpp
@@ -347,7 +347,7 @@ KResultOr<FlatPtr> Process::sys$mprotect(Userspace<void*> 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<FlatPtr> Process::sys$mprotect(Userspace<void*> 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<FlatPtr> Process::sys$mprotect(Userspace<void*> 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;