Kernel: AnonymousVMObject::create_for_physical_range() should fail more

Previously it was not possible for this function to fail. You could
exploit this by triggering the creation of a VMObject whose physical
memory range would wrap around the 32-bit limit.

It was quite easy to map kernel memory into userspace and read/write
whatever you wanted in it.

Test: Kernel/bxvga-mmap-kernel-into-userspace.cpp

5 files changed, 17 insertions, 6 deletions

diff --git a/Kernel/Devices/BXVGADevice.cpp b/Kernel/Devices/BXVGADevice.cpp
index 714096f586..47730d9936 100644
--- a/Kernel/Devices/BXVGADevice.cpp
+++ b/Kernel/Devices/BXVGADevice.cpp
@@ -116,10 +116,12 @@ KResultOr<Region*> BXVGADevice::mmap(Process& process, FileDescription&, Virtual
     ASSERT(offset == 0);
     ASSERT(size == framebuffer_size_in_bytes());
     auto vmobject = AnonymousVMObject::create_for_physical_range(m_framebuffer_address, framebuffer_size_in_bytes());
+    if (!vmobject)
+        return KResult(-ENOMEM);
     auto* region = process.allocate_region_with_vmobject(
         preferred_vaddr,
         framebuffer_size_in_bytes(),
-        move(vmobject),
+        vmobject.release_nonnull(),
         0,
         "BXVGA Framebuffer",
         prot);
diff --git a/Kernel/Devices/MBVGADevice.cpp b/Kernel/Devices/MBVGADevice.cpp
index c8ca9f6177..6ec0d1aa87 100644
--- a/Kernel/Devices/MBVGADevice.cpp
+++ b/Kernel/Devices/MBVGADevice.cpp
@@ -55,10 +55,12 @@ KResultOr<Region*> MBVGADevice::mmap(Process& process, FileDescription&, Virtual
     ASSERT(offset == 0);
     ASSERT(size == framebuffer_size_in_bytes());
     auto vmobject = AnonymousVMObject::create_for_physical_range(m_framebuffer_address, framebuffer_size_in_bytes());
+    if (!vmobject)
+        return KResult(-ENOMEM);
     auto* region = process.allocate_region_with_vmobject(
         preferred_vaddr,
         framebuffer_size_in_bytes(),
-        move(vmobject),
+        vmobject.release_nonnull(),
         0,
         "MBVGA Framebuffer",
         prot);
diff --git a/Kernel/VM/AnonymousVMObject.cpp b/Kernel/VM/AnonymousVMObject.cpp
index 1b1dfe3ed4..60e905f4ec 100644
--- a/Kernel/VM/AnonymousVMObject.cpp
+++ b/Kernel/VM/AnonymousVMObject.cpp
@@ -32,8 +32,12 @@ NonnullRefPtr<AnonymousVMObject> AnonymousVMObject::create_with_size(size_t size
     return adopt(*new AnonymousVMObject(size));
 }
 
-NonnullRefPtr<AnonymousVMObject> AnonymousVMObject::create_for_physical_range(PhysicalAddress paddr, size_t size)
+RefPtr<AnonymousVMObject> AnonymousVMObject::create_for_physical_range(PhysicalAddress paddr, size_t size)
 {
+    if (paddr.offset(size) < paddr) {
+        dbg() << "Shenanigans! create_for_physical_range(" << paddr << ", " << size << ") would wrap around";
+        return nullptr;
+    }
     return adopt(*new AnonymousVMObject(paddr, size));
 }
 
diff --git a/Kernel/VM/AnonymousVMObject.h b/Kernel/VM/AnonymousVMObject.h
index 5af312f4de..29a4d19538 100644
--- a/Kernel/VM/AnonymousVMObject.h
+++ b/Kernel/VM/AnonymousVMObject.h
@@ -34,7 +34,7 @@ public:
     virtual ~AnonymousVMObject() override;
 
     static NonnullRefPtr<AnonymousVMObject> create_with_size(size_t);
-    static NonnullRefPtr<AnonymousVMObject> create_for_physical_range(PhysicalAddress, size_t);
+    static RefPtr<AnonymousVMObject> create_for_physical_range(PhysicalAddress, size_t);
     static NonnullRefPtr<AnonymousVMObject> create_with_physical_page(PhysicalPage&);
     virtual NonnullRefPtr<VMObject> clone() override;
 
diff --git a/Kernel/VM/MemoryManager.cpp b/Kernel/VM/MemoryManager.cpp
index db877a128e..9053f201f3 100644
--- a/Kernel/VM/MemoryManager.cpp
+++ b/Kernel/VM/MemoryManager.cpp
@@ -313,11 +313,14 @@ OwnPtr<Region> MemoryManager::allocate_kernel_region(PhysicalAddress paddr, size
     ASSERT(!(size % PAGE_SIZE));
     auto range = kernel_page_directory().range_allocator().allocate_anywhere(size);
     ASSERT(range.is_valid());
+    auto vmobject = AnonymousVMObject::create_for_physical_range(paddr, size);
+    if (!vmobject)
+        return nullptr;
     OwnPtr<Region> region;
     if (user_accessible)
-        region = Region::create_user_accessible(range, AnonymousVMObject::create_for_physical_range(paddr, size), 0, name, access, cacheable);
+        region = Region::create_user_accessible(range, vmobject.release_nonnull(), 0, name, access, cacheable);
     else
-        region = Region::create_kernel_only(range, AnonymousVMObject::create_for_physical_range(paddr, size), 0, name, access, cacheable);
+        region = Region::create_kernel_only(range, vmobject.release_nonnull(), 0, name, access, cacheable);
     region->map(kernel_page_directory());
     return region;
 }