From af3d3c5c4afdbb5008e476283c678e73bb44346b Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 29 Jan 2021 14:38:49 +0100 Subject: [PATCH] Kernel: Enforce W^X more strictly (like PaX MPROTECT) This patch adds enforcement of two new rules: - Memory that was previously writable cannot become executable - Memory that was previously executable cannot become writable Unfortunately we have to make an exception for text relocations in the dynamic loader. Since those necessitate writing into a private copy of library code, we allow programs to transition from RW to RX under very specific conditions. See the implementation of sys$mprotect()'s should_make_executable_exception_for_dynamic_loader() for details. --- Kernel/Syscalls/mmap.cpp | 95 +++++++++++++++++++++++++++++++++++----- Kernel/VM/Region.cpp | 2 +- Kernel/VM/Region.h | 22 +++++++++- 3 files changed, 106 insertions(+), 13 deletions(-) diff --git a/Kernel/Syscalls/mmap.cpp b/Kernel/Syscalls/mmap.cpp index 47c2ad65ca4..5e548998429 100644 --- a/Kernel/Syscalls/mmap.cpp +++ b/Kernel/Syscalls/mmap.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2020, Andreas Kling + * Copyright (c) 2018-2021, Andreas Kling * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -33,25 +33,91 @@ #include #include #include +#include namespace Kernel { -static bool validate_mmap_prot(int prot, bool map_stack) +static bool should_make_executable_exception_for_dynamic_loader(bool make_readable, bool make_writable, bool make_executable, const Region& region) { - bool readable = prot & PROT_READ; - bool writable = prot & PROT_WRITE; - bool executable = prot & PROT_EXEC; + // Normally we don't allow W -> X transitions, but we have to make an exception + // for the dynamic loader, which needs to do this after performing text relocations. - if (writable && executable) + // FIXME: Investigate whether we could get rid of all text relocations entirely. + + // The exception is only made if all the following criteria is fulfilled: + + // This exception has not been made for the same region already + if (region.has_made_executable_exception_for_dynamic_loader()) + return false; + + // The region must be RW + if (!(region.is_readable() && region.is_writable() && !region.is_executable())) + return false; + + // The region wants to become RX + if (!(make_readable && !make_writable && make_executable)) + return false; + + // The region is backed by a file + if (!region.vmobject().is_inode()) + return false; + + // The file mapping is private, not shared (no relocations in a shared mapping!) + if (!region.vmobject().is_private_inode()) + return false; + + Elf32_Ehdr header; + if (!copy_from_user(&header, region.vaddr().as_ptr(), sizeof(header))) + return false; + + auto& inode = static_cast(region.vmobject()); + + // The file is a valid ELF binary + if (!ELF::validate_elf_header(header, inode.size())) + return false; + + // The file is an ELF shared object + if (header.e_type != ET_DYN) + return false; + + // FIXME: Are there any additional checks/validations we could do here? + return true; +} + +static bool validate_mmap_prot(int prot, bool map_stack, const Region* region = nullptr, bool* is_making_executable_exception_for_dynamic_loader = nullptr) +{ + if (is_making_executable_exception_for_dynamic_loader) + *is_making_executable_exception_for_dynamic_loader = false; + + bool make_readable = prot & PROT_READ; + bool make_writable = prot & PROT_WRITE; + bool make_executable = prot & PROT_EXEC; + + if (make_writable && make_executable) return false; if (map_stack) { - if (executable) + if (make_executable) return false; - if (!readable || !writable) + if (!make_readable || !make_writable) return false; } + if (region) { + if (make_writable && region->has_been_executable()) + return false; + + if (make_executable && region->has_been_writable()) { + if (should_make_executable_exception_for_dynamic_loader(make_readable, make_writable, make_executable, *region)) { + ASSERT(is_making_executable_exception_for_dynamic_loader); + *is_making_executable_exception_for_dynamic_loader = true; + return true; + } + + return false; + } + } + return true; } @@ -216,7 +282,8 @@ int Process::sys$mprotect(void* addr, size_t size, int prot) if (auto* whole_region = find_region_from_range(range_to_mprotect)) { if (!whole_region->is_mmap()) return -EPERM; - if (!validate_mmap_prot(prot, whole_region->is_stack())) + bool is_making_executable_exception_for_dynamic_loader = false; + if (!validate_mmap_prot(prot, whole_region->is_stack(), whole_region, &is_making_executable_exception_for_dynamic_loader)) return -EINVAL; if (whole_region->access() == prot_to_region_access_flags(prot)) return 0; @@ -227,6 +294,10 @@ int Process::sys$mprotect(void* addr, size_t size, int prot) whole_region->set_readable(prot & PROT_READ); whole_region->set_writable(prot & PROT_WRITE); whole_region->set_executable(prot & PROT_EXEC); + + if (is_making_executable_exception_for_dynamic_loader) + whole_region->set_has_made_executable_exception_for_dynamic_loader(); + whole_region->remap(); return 0; } @@ -235,7 +306,8 @@ int Process::sys$mprotect(void* addr, size_t size, int prot) if (auto* old_region = find_region_containing(range_to_mprotect)) { if (!old_region->is_mmap()) return -EPERM; - if (!validate_mmap_prot(prot, old_region->is_stack())) + bool is_making_executable_exception_for_dynamic_loader = false; + if (!validate_mmap_prot(prot, old_region->is_stack(), old_region, &is_making_executable_exception_for_dynamic_loader)) return -EINVAL; if (old_region->access() == prot_to_region_access_flags(prot)) return 0; @@ -254,6 +326,9 @@ int Process::sys$mprotect(void* addr, size_t size, int prot) new_region.set_writable(prot & PROT_WRITE); new_region.set_executable(prot & PROT_EXEC); + if (is_making_executable_exception_for_dynamic_loader) + new_region.set_has_made_executable_exception_for_dynamic_loader(); + // Unmap the old region here, specifying that we *don't* want the VM deallocated. old_region->unmap(Region::ShouldDeallocateVirtualMemoryRange::No); deallocate_region(*old_region); diff --git a/Kernel/VM/Region.cpp b/Kernel/VM/Region.cpp index 54f216dc90b..b21b2d9cd80 100644 --- a/Kernel/VM/Region.cpp +++ b/Kernel/VM/Region.cpp @@ -44,7 +44,7 @@ Region::Region(const Range& range, NonnullRefPtr vmobject, size_t offs , m_offset_in_vmobject(offset_in_vmobject) , m_vmobject(move(vmobject)) , m_name(name) - , m_access(access) + , m_access(access | ((access & 0x7) << 4)) , m_shared(shared) , m_cacheable(cacheable) , m_kernel(kernel) diff --git a/Kernel/VM/Region.h b/Kernel/VM/Region.h index 87b0f750041..93385ad8870 100644 --- a/Kernel/VM/Region.h +++ b/Kernel/VM/Region.h @@ -50,10 +50,16 @@ class Region final MAKE_SLAB_ALLOCATED(Region) public: - enum Access { + // 76543210 + // eXWR xwr + enum Access : u8 { Read = 1, Write = 2, Execute = 4, + HasBeenReadable = 16, + HasBeenWritable = 32, + HasBeenExecutable = 64, + HasMadeExecutableExceptionForDynamicLoader = 128, }; static NonnullOwnPtr create_user_accessible(Process*, const Range&, NonnullRefPtr, size_t offset_in_vmobject, const StringView& name, u8 access, bool cacheable, bool shared); @@ -67,6 +73,18 @@ public: bool is_readable() const { return m_access & Access::Read; } bool is_writable() const { return m_access & Access::Write; } bool is_executable() const { return m_access & Access::Execute; } + + bool has_been_readable() const { return m_access & Access::HasBeenReadable; } + bool has_been_writable() const { return m_access & Access::HasBeenWritable; } + bool has_been_executable() const { return m_access & Access::HasBeenExecutable; } + + bool has_made_executable_exception_for_dynamic_loader() const { return m_access & Access::HasMadeExecutableExceptionForDynamicLoader; } + void set_has_made_executable_exception_for_dynamic_loader() + { + ASSERT(!has_made_executable_exception_for_dynamic_loader()); + m_access |= Access::HasMadeExecutableExceptionForDynamicLoader; + } + bool is_cacheable() const { return m_cacheable; } const String& name() const { return m_name; } unsigned access() const { return m_access; } @@ -245,7 +263,7 @@ private: void set_access_bit(Access access, bool b) { if (b) - m_access |= access; + m_access |= access | (access << 4); else m_access &= ~access; }