From 8fbdda5a2dae61d336e5fabcc6b6adf5c8efa308 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 28 Feb 2020 20:47:27 +0100 Subject: [PATCH] Kernel: Implement basic support for sys$mmap() with MAP_PRIVATE You can now mmap a file as private and writable, and the changes you make will only be visible to you. This works because internally a MAP_PRIVATE region is backed by a unique PrivateInodeVMObject instead of using the globally shared SharedInodeVMObject like we always did before. :^) Fixes #1045. --- Kernel/Devices/BXVGADevice.cpp | 4 ++- Kernel/Devices/BXVGADevice.h | 2 +- Kernel/Devices/MBVGADevice.cpp | 4 ++- Kernel/Devices/MBVGADevice.h | 2 +- Kernel/FileSystem/File.cpp | 2 +- Kernel/FileSystem/File.h | 2 +- Kernel/FileSystem/FileDescription.cpp | 4 +-- Kernel/FileSystem/FileDescription.h | 2 +- Kernel/FileSystem/InodeFile.cpp | 12 ++++++-- Kernel/FileSystem/InodeFile.h | 2 +- Kernel/Process.cpp | 40 ++++++++++++++------------- 11 files changed, 45 insertions(+), 31 deletions(-) diff --git a/Kernel/Devices/BXVGADevice.cpp b/Kernel/Devices/BXVGADevice.cpp index 71a57e9c92..807456b21e 100644 --- a/Kernel/Devices/BXVGADevice.cpp +++ b/Kernel/Devices/BXVGADevice.cpp @@ -166,9 +166,11 @@ u32 BXVGADevice::find_framebuffer_address() return framebuffer_address; } -KResultOr BXVGADevice::mmap(Process& process, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot) +KResultOr BXVGADevice::mmap(Process& process, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot, bool shared) { REQUIRE_PROMISE(video); + if (!shared) + return KResult(-ENODEV); ASSERT(offset == 0); ASSERT(size == framebuffer_size_in_bytes()); auto vmobject = AnonymousVMObject::create_for_physical_range(m_framebuffer_address, framebuffer_size_in_bytes()); diff --git a/Kernel/Devices/BXVGADevice.h b/Kernel/Devices/BXVGADevice.h index 5ce8758f52..889a795f21 100644 --- a/Kernel/Devices/BXVGADevice.h +++ b/Kernel/Devices/BXVGADevice.h @@ -41,7 +41,7 @@ public: BXVGADevice(); virtual int ioctl(FileDescription&, unsigned request, unsigned arg) override; - virtual KResultOr mmap(Process&, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t, int prot) override; + virtual KResultOr mmap(Process&, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t, int prot, bool shared) override; private: virtual const char* class_name() const override { return "BXVGA"; } diff --git a/Kernel/Devices/MBVGADevice.cpp b/Kernel/Devices/MBVGADevice.cpp index 6acacddbc6..31d17f1ead 100644 --- a/Kernel/Devices/MBVGADevice.cpp +++ b/Kernel/Devices/MBVGADevice.cpp @@ -51,9 +51,11 @@ MBVGADevice::MBVGADevice(PhysicalAddress addr, int pitch, int width, int height) s_the = this; } -KResultOr MBVGADevice::mmap(Process& process, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot) +KResultOr MBVGADevice::mmap(Process& process, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot, bool shared) { REQUIRE_PROMISE(video); + if (!shared) + return KResult(-ENODEV); ASSERT(offset == 0); ASSERT(size == framebuffer_size_in_bytes()); auto vmobject = AnonymousVMObject::create_for_physical_range(m_framebuffer_address, framebuffer_size_in_bytes()); diff --git a/Kernel/Devices/MBVGADevice.h b/Kernel/Devices/MBVGADevice.h index c145a81c8b..275061eeb8 100644 --- a/Kernel/Devices/MBVGADevice.h +++ b/Kernel/Devices/MBVGADevice.h @@ -41,7 +41,7 @@ public: MBVGADevice(PhysicalAddress addr, int pitch, int width, int height); virtual int ioctl(FileDescription&, unsigned request, unsigned arg) override; - virtual KResultOr mmap(Process&, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t, int prot) override; + virtual KResultOr mmap(Process&, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t, int prot, bool shared) override; private: virtual const char* class_name() const override { return "MBVGA"; } diff --git a/Kernel/FileSystem/File.cpp b/Kernel/FileSystem/File.cpp index 04d0e2a4a5..8903220809 100644 --- a/Kernel/FileSystem/File.cpp +++ b/Kernel/FileSystem/File.cpp @@ -54,7 +54,7 @@ int File::ioctl(FileDescription&, unsigned, unsigned) return -ENOTTY; } -KResultOr File::mmap(Process&, FileDescription&, VirtualAddress, size_t, size_t, int) +KResultOr File::mmap(Process&, FileDescription&, VirtualAddress, size_t, size_t, int, bool) { return KResult(-ENODEV); } diff --git a/Kernel/FileSystem/File.h b/Kernel/FileSystem/File.h index 3779ef8510..cf1d065d65 100644 --- a/Kernel/FileSystem/File.h +++ b/Kernel/FileSystem/File.h @@ -77,7 +77,7 @@ public: virtual ssize_t read(FileDescription&, u8*, ssize_t) = 0; virtual ssize_t write(FileDescription&, const u8*, ssize_t) = 0; virtual int ioctl(FileDescription&, unsigned request, unsigned arg); - virtual KResultOr mmap(Process&, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot); + virtual KResultOr mmap(Process&, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot, bool shared); virtual String absolute_path(const FileDescription&) const = 0; diff --git a/Kernel/FileSystem/FileDescription.cpp b/Kernel/FileSystem/FileDescription.cpp index 1bd1659563..f159f30d92 100644 --- a/Kernel/FileSystem/FileDescription.cpp +++ b/Kernel/FileSystem/FileDescription.cpp @@ -281,10 +281,10 @@ InodeMetadata FileDescription::metadata() const return {}; } -KResultOr FileDescription::mmap(Process& process, VirtualAddress vaddr, size_t offset, size_t size, int prot) +KResultOr FileDescription::mmap(Process& process, VirtualAddress vaddr, size_t offset, size_t size, int prot, bool shared) { LOCKER(m_lock); - return m_file->mmap(process, *this, vaddr, offset, size, prot); + return m_file->mmap(process, *this, vaddr, offset, size, prot, shared); } KResult FileDescription::truncate(u64 length) diff --git a/Kernel/FileSystem/FileDescription.h b/Kernel/FileSystem/FileDescription.h index 5ad052cf82..648e6f9366 100644 --- a/Kernel/FileSystem/FileDescription.h +++ b/Kernel/FileSystem/FileDescription.h @@ -109,7 +109,7 @@ public: Custody* custody() { return m_custody.ptr(); } const Custody* custody() const { return m_custody.ptr(); } - KResultOr mmap(Process&, VirtualAddress, size_t offset, size_t, int prot); + KResultOr mmap(Process&, VirtualAddress, size_t offset, size_t, int prot, bool shared); bool is_blocking() const { return m_is_blocking; } void set_blocking(bool b) { m_is_blocking = b; } diff --git a/Kernel/FileSystem/InodeFile.cpp b/Kernel/FileSystem/InodeFile.cpp index 1f4d70c127..399ae4a11a 100644 --- a/Kernel/FileSystem/InodeFile.cpp +++ b/Kernel/FileSystem/InodeFile.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include namespace Kernel { @@ -60,11 +61,18 @@ ssize_t InodeFile::write(FileDescription& description, const u8* data, ssize_t c return nwritten; } -KResultOr InodeFile::mmap(Process& process, FileDescription& description, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot) +KResultOr InodeFile::mmap(Process& process, FileDescription& description, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot, bool shared) { ASSERT(offset == 0); // FIXME: If PROT_EXEC, check that the underlying file system isn't mounted noexec. - auto* region = process.allocate_region_with_vmobject(preferred_vaddr, size, SharedInodeVMObject::create_with_inode(inode()), offset, description.absolute_path(), prot); + RefPtr vmobject; + if (shared) + vmobject = SharedInodeVMObject::create_with_inode(inode()); + else + vmobject = PrivateInodeVMObject::create_with_inode(inode()); + if (!vmobject) + return KResult(-ENOMEM); + auto* region = process.allocate_region_with_vmobject(preferred_vaddr, size, *vmobject, offset, description.absolute_path(), prot); if (!region) return KResult(-ENOMEM); return region; diff --git a/Kernel/FileSystem/InodeFile.h b/Kernel/FileSystem/InodeFile.h index f83e4c8dbc..7aaca304e4 100644 --- a/Kernel/FileSystem/InodeFile.h +++ b/Kernel/FileSystem/InodeFile.h @@ -49,7 +49,7 @@ public: virtual ssize_t read(FileDescription&, u8*, ssize_t) override; virtual ssize_t write(FileDescription&, const u8*, ssize_t) override; - virtual KResultOr mmap(Process&, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot) override; + virtual KResultOr mmap(Process&, FileDescription&, VirtualAddress preferred_vaddr, size_t offset, size_t size, int prot, bool shared) override; virtual String absolute_path(const FileDescription&) const override; diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 4d31f72f8d..70092e2e67 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -321,19 +321,22 @@ static bool validate_mmap_prot(int prot, bool map_stack) return true; } -static bool validate_inode_mmap_prot(const Process& process, int prot, const Inode& inode) +static bool validate_inode_mmap_prot(const Process& process, int prot, const Inode& inode, bool map_shared) { auto metadata = inode.metadata(); - if ((prot & PROT_WRITE) && !metadata.may_write(process)) - return false; if ((prot & PROT_READ) && !metadata.may_read(process)) return false; - InterruptDisabler disabler; - if (inode.shared_vmobject()) { - if ((prot & PROT_EXEC) && inode.shared_vmobject()->writable_mappings()) - return false; - if ((prot & PROT_WRITE) && inode.shared_vmobject()->executable_mappings()) + + if (map_shared) { + if ((prot & PROT_WRITE) && !metadata.may_write(process)) return false; + InterruptDisabler disabler; + if (inode.shared_vmobject()) { + if ((prot & PROT_EXEC) && inode.shared_vmobject()->writable_mappings()) + return false; + if ((prot & PROT_WRITE) && inode.shared_vmobject()->executable_mappings()) + return false; + } } return true; } @@ -433,9 +436,6 @@ void* Process::sys$mmap(const Syscall::SC_mmap_params* user_params) return (void*)-EINVAL; if (static_cast(offset) & ~PAGE_MASK) return (void*)-EINVAL; - // FIXME: Implement MAP_PRIVATE for FileDescription-backed mmap - if (map_private) - return (void*)-ENOTSUP; auto description = file_description(fd); if (!description) return (void*)-EBADF; @@ -443,18 +443,20 @@ void* Process::sys$mmap(const Syscall::SC_mmap_params* user_params) return (void*)-ENODEV; if ((prot & PROT_READ) && !description->is_readable()) return (void*)-EACCES; - if ((prot & PROT_WRITE) && !description->is_writable()) - return (void*)-EACCES; - if (description->inode()) { - if (!validate_inode_mmap_prot(*this, prot, *description->inode())) + if (map_shared) { + if ((prot & PROT_WRITE) && !description->is_writable()) return (void*)-EACCES; } - auto region_or_error = description->mmap(*this, VirtualAddress(addr), static_cast(offset), size, prot); + if (description->inode()) { + if (!validate_inode_mmap_prot(*this, prot, *description->inode(), map_shared)) + return (void*)-EACCES; + } + auto region_or_error = description->mmap(*this, VirtualAddress(addr), static_cast(offset), size, prot, map_shared); if (region_or_error.is_error()) { // Fail if MAP_FIXED or address is 0, retry otherwise if (map_fixed || addr == 0) return (void*)(int)region_or_error.error(); - region_or_error = description->mmap(*this, {}, static_cast(offset), size, prot); + region_or_error = description->mmap(*this, {}, static_cast(offset), size, prot, map_shared); } if (region_or_error.is_error()) return (void*)(int)region_or_error.error(); @@ -537,7 +539,7 @@ int Process::sys$mprotect(void* addr, size_t size, int prot) if (whole_region->access() == prot_to_region_access_flags(prot)) return 0; if (whole_region->vmobject().is_inode() - && !validate_inode_mmap_prot(*this, prot, static_cast(whole_region->vmobject()).inode())) { + && !validate_inode_mmap_prot(*this, prot, static_cast(whole_region->vmobject()).inode(), whole_region->is_shared())) { return -EACCES; } whole_region->set_readable(prot & PROT_READ); @@ -556,7 +558,7 @@ int Process::sys$mprotect(void* addr, size_t size, int prot) if (old_region->access() == prot_to_region_access_flags(prot)) return 0; if (old_region->vmobject().is_inode() - && !validate_inode_mmap_prot(*this, prot, static_cast(old_region->vmobject()).inode())) { + && !validate_inode_mmap_prot(*this, prot, static_cast(old_region->vmobject()).inode(), old_region->is_shared())) { return -EACCES; }