From 931e4b7f5ec8a5ced69c3789ecdcf0772cbee7ef Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 15 Dec 2019 20:11:57 +0100 Subject: [PATCH] Kernel+SystemMonitor: Prevent userspace access to process ELF image Every process keeps its own ELF executable mapped in memory in case we need to do symbol lookup (for backtraces, etc.) Until now, it was mapped in a way that made it accessible to the program, despite the program not having mapped it itself. I don't really see a need for userspace to have access to this right now, so let's lock things down a little bit. This patch makes it inaccessible to userspace and exposes that fact through /proc/PID/vm (per-region "user_accessible" flag.) --- Applications/SystemMonitor/ProcessMemoryMapWidget.cpp | 2 ++ Kernel/FileSystem/ProcFS.cpp | 1 + Kernel/Process.cpp | 3 +++ Kernel/VM/Region.cpp | 3 --- Kernel/VM/Region.h | 1 + 5 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Applications/SystemMonitor/ProcessMemoryMapWidget.cpp b/Applications/SystemMonitor/ProcessMemoryMapWidget.cpp index c624cf01d2..860c86aa1d 100644 --- a/Applications/SystemMonitor/ProcessMemoryMapWidget.cpp +++ b/Applications/SystemMonitor/ProcessMemoryMapWidget.cpp @@ -20,6 +20,8 @@ ProcessMemoryMapWidget::ProcessMemoryMapWidget(GWidget* parent) pid_vm_fields.empend("amount_resident", "Resident", TextAlignment::CenterRight); pid_vm_fields.empend("Access", TextAlignment::CenterLeft, [](auto& object) { StringBuilder builder; + if (!object.get("user_accessible").to_bool()) + builder.append('K'); if (object.get("readable").to_bool()) builder.append('R'); if (object.get("writable").to_bool()) diff --git a/Kernel/FileSystem/ProcFS.cpp b/Kernel/FileSystem/ProcFS.cpp index 0195717083..d8e0002711 100644 --- a/Kernel/FileSystem/ProcFS.cpp +++ b/Kernel/FileSystem/ProcFS.cpp @@ -265,6 +265,7 @@ Optional procfs$pid_vm(InodeIdentifier identifier) region_object.add("writable", region.is_writable()); region_object.add("stack", region.is_stack()); region_object.add("shared", region.is_shared()); + region_object.add("user_accessible", region.is_user_accessible()); region_object.add("purgeable", region.vmobject().is_purgeable()); if (region.vmobject().is_purgeable()) { region_object.add("volatile", static_cast(region.vmobject()).is_volatile()); diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 7dfa2fc2d9..93b661ce8e 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -570,6 +570,9 @@ int Process::do_exec(String path, Vector arguments, Vector envir entry_eip = loader->entry().get(); } + region->set_user_accessible(false); + region->remap(); + m_elf_loader = move(loader); m_executable = description->custody(); diff --git a/Kernel/VM/Region.cpp b/Kernel/VM/Region.cpp index 822d419532..3148be84b7 100644 --- a/Kernel/VM/Region.cpp +++ b/Kernel/VM/Region.cpp @@ -54,9 +54,6 @@ NonnullOwnPtr Region::clone() { ASSERT(current); - // NOTE: Kernel-only regions should never be cloned. - ASSERT(is_user_accessible()); - // FIXME: What should we do for privately mapped InodeVMObjects? if (m_shared || vmobject().is_inode()) { ASSERT(!m_stack); diff --git a/Kernel/VM/Region.h b/Kernel/VM/Region.h index 9092867bc1..eb76b3accb 100644 --- a/Kernel/VM/Region.h +++ b/Kernel/VM/Region.h @@ -57,6 +57,7 @@ public: void set_mmap(bool mmap) { m_mmap = mmap; } bool is_user_accessible() const { return m_user_accessible; } + void set_user_accessible(bool b) { m_user_accessible = b; } PageFaultResponse handle_fault(const PageFault&);