Core: Handle various edge cases related to executable permissions. (#3660)

* Fix flag handling on Windows

Fixes a weird homebrew kalaposfos made

* Fix backing protects

Windows requires that protections on areas committed through MapViewOfFile functions are less than the original mapping.
The best way to make sure everything works is to VirtualProtect the code area with the requested protection instead of applying prot directly.

* Fix error code for sceKernelMapDirectMemory2

Real hardware returns EINVAL instead of EACCES here

* Fix prot setting in ProtectBytes

* Handle some extra protection-related edge cases.

Real hardware treats read and write as separate perms, but appends read if you call with write-only (this is visible in VirtualQuery calls)

Additionally, execute permissions are ignored when protecting dmem mappings.

* Properly handle exec permission behavior for memory pools

Calling sceKernelMemoryPoolCommit with executable permissions returns EINVAL, mprotect on pooled mappings ignores the exec protection.

* Clang

* Allow execution protection for direct memory

Further hardware tests show that the dmem area is actually executable, this permission is just hidden from the end user.

* Clang

* More descriptive assert message

* Align address and size in mmap

Like most POSIX functions, mmap aligns address down to the nearest page boundary, and aligns address up to the nearest page boundary.
Since mmap is the only memory mapping function that doesn't error early on misaligned length or size, handle the alignment in the libkernel code.

* Clang

* Fix valid flags

After changing the value, games that specify just CpuWrite would hit the error return.

* Fix prot conversion functions

The True(bool) function returns true whenever value is greater than 0. While this rarely manifested before because of our wrongly defined CpuReadWrite prot, it's now causing trouble with the corrected values.
Technically this could've also caused trouble with games mapping GpuRead permissions, but that seems to be a rare enough use case that I guess it never happened?

I've also added a warning for the case where `write & !read`, since we don't properly handle write-only permissions, and I'm not entirely sure what it would take to deal with that.

* Fix some lingering dmem issues

ReleaseDirectMemory was always unmapping with the size parameter, which could cause it to unmap too much. Since multiple mappings can reference the same dmem area, I've calculated how much of each VMA we're supposed to unmap.
Additionally, I've adjusted the logic for carving out the free dmem area to properly work if ReleaseDirectMemory is called over multiple dmem areas.

Finally, I've patched a bug with my code in UnmapMemory.

src/core/address_space.cpp

@@ -32,13 +32,34 @@ static constexpr size_t BackingSize = ORBIS_KERNEL_TOTAL_MEM_DEV_PRO;
 #ifdef _WIN32
 
 [[nodiscard]] constexpr u64 ToWindowsProt(Core::MemoryProt prot) {
-    if (True(prot & Core::MemoryProt::CpuReadWrite) ||
-        True(prot & Core::MemoryProt::GpuReadWrite)) {
-        return PAGE_READWRITE;
-    } else if (True(prot & Core::MemoryProt::CpuRead) || True(prot & Core::MemoryProt::GpuRead)) {
-        return PAGE_READONLY;
+    const bool read =
+        True(prot & Core::MemoryProt::CpuRead) || True(prot & Core::MemoryProt::GpuRead);
+    const bool write =
+        True(prot & Core::MemoryProt::CpuWrite) || True(prot & Core::MemoryProt::GpuWrite);
+    const bool execute = True(prot & Core::MemoryProt::CpuExec);
+
+    if (write && !read) {
+        // While write-only CPU mappings aren't possible, write-only GPU mappings are.
+        LOG_WARNING(Core, "Converting write-only mapping to read-write");
+    }
+
+    // All cases involving execute permissions have separate permissions.
+    if (execute) {
+        if (write) {
+            return PAGE_EXECUTE_READWRITE;
+        } else if (read && !write) {
+            return PAGE_EXECUTE_READ;
+        } else {
+            return PAGE_EXECUTE;
+        }
     } else {
-        return PAGE_NOACCESS;
+        if (write) {
+            return PAGE_READWRITE;
+        } else if (read && !write) {
+            return PAGE_READONLY;
+        } else {
+            return PAGE_NOACCESS;
+        }
     }
 }
 
@@ -155,7 +176,12 @@ struct AddressSpace::Impl {
                 ASSERT_MSG(ret, "VirtualProtect failed. {}", Common::GetLastErrorMsg());
             } else {
                 ptr = MapViewOfFile3(backing, process, reinterpret_cast<PVOID>(virtual_addr),
-                                     phys_addr, size, MEM_REPLACE_PLACEHOLDER, prot, nullptr, 0);
+                                     phys_addr, size, MEM_REPLACE_PLACEHOLDER,
+                                     PAGE_EXECUTE_READWRITE, nullptr, 0);
+                ASSERT_MSG(ptr, "MapViewOfFile3 failed. {}", Common::GetLastErrorMsg());
+                DWORD resultvar;
+                bool ret = VirtualProtect(ptr, size, prot, &resultvar);
+                ASSERT_MSG(ret, "VirtualProtect failed. {}", Common::GetLastErrorMsg());
             }
         } else {
             ptr =
@@ -297,17 +323,33 @@ struct AddressSpace::Impl {
     void Protect(VAddr virtual_addr, size_t size, bool read, bool write, bool execute) {
         DWORD new_flags{};
 
-        if (read && write && execute) {
-            new_flags = PAGE_EXECUTE_READWRITE;
-        } else if (read && write) {
-            new_flags = PAGE_READWRITE;
-        } else if (read && !write) {
-            new_flags = PAGE_READONLY;
-        } else if (execute && !read && !write) {
-            new_flags = PAGE_EXECUTE;
-        } else if (!read && !write && !execute) {
-            new_flags = PAGE_NOACCESS;
+        if (write && !read) {
+            // While write-only CPU protection isn't possible, write-only GPU protection is.
+            LOG_WARNING(Core, "Converting write-only protection to read-write");
+        }
+
+        // All cases involving execute permissions have separate permissions.
+        if (execute) {
+            // If there's some form of write protection requested, provide read-write permissions.
+            if (write) {
+                new_flags = PAGE_EXECUTE_READWRITE;
+            } else if (read && !write) {
+                new_flags = PAGE_EXECUTE_READ;
+            } else {
+                new_flags = PAGE_EXECUTE;
+            }
         } else {
+            if (write) {
+                new_flags = PAGE_READWRITE;
+            } else if (read && !write) {
+                new_flags = PAGE_READONLY;
+            } else {
+                new_flags = PAGE_NOACCESS;
+            }
+        }
+
+        // If no flags are assigned, then something's gone wrong.
+        if (new_flags == 0) {
             LOG_CRITICAL(Common_Memory,
                          "Unsupported protection flag combination for address {:#x}, size {}, "
                          "read={}, write={}, execute={}",
@@ -327,7 +369,7 @@ struct AddressSpace::Impl {
             DWORD old_flags{};
             if (!VirtualProtectEx(process, LPVOID(range_addr), range_size, new_flags, &old_flags)) {
                 UNREACHABLE_MSG(
-                    "Failed to change virtual memory protection for address {:#x}, size {}",
+                    "Failed to change virtual memory protection for address {:#x}, size {:#x}",
                     range_addr, range_size);
             }
         }
@@ -357,21 +399,34 @@ enum PosixPageProtection {
 };
 
 [[nodiscard]] constexpr PosixPageProtection ToPosixProt(Core::MemoryProt prot) {
-    if (True(prot & Core::MemoryProt::CpuReadWrite) ||
-        True(prot & Core::MemoryProt::GpuReadWrite)) {
-        if (True(prot & Core::MemoryProt::CpuExec)) {
+    const bool read =
+        True(prot & Core::MemoryProt::CpuRead) || True(prot & Core::MemoryProt::GpuRead);
+    const bool write =
+        True(prot & Core::MemoryProt::CpuWrite) || True(prot & Core::MemoryProt::GpuWrite);
+    const bool execute = True(prot & Core::MemoryProt::CpuExec);
+
+    if (write && !read) {
+        // While write-only CPU mappings aren't possible, write-only GPU mappings are.
+        LOG_WARNING(Core, "Converting write-only mapping to read-write");
+    }
+
+    // All cases involving execute permissions have separate permissions.
+    if (execute) {
+        if (write) {
             return PAGE_EXECUTE_READWRITE;
-        } else {
-            return PAGE_READWRITE;
-        }
-    } else if (True(prot & Core::MemoryProt::CpuRead) || True(prot & Core::MemoryProt::GpuRead)) {
-        if (True(prot & Core::MemoryProt::CpuExec)) {
+        } else if (read && !write) {
             return PAGE_EXECUTE_READ;
         } else {
-            return PAGE_READONLY;
+            return PAGE_EXECUTE;
         }
     } else {
-        return PAGE_NOACCESS;
+        if (write) {
+            return PAGE_READWRITE;
+        } else if (read && !write) {
+            return PAGE_READONLY;
+        } else {
+            return PAGE_NOACCESS;
+        }
     }
 }
 

src/core/libraries/kernel/memory.cpp

@@ -237,7 +237,7 @@ s32 PS4_SYSV_ABI sceKernelMapDirectMemory2(void** addr, u64 len, s32 type, s32 p
     const auto mem_prot = static_cast<Core::MemoryProt>(prot);
     if (True(mem_prot & Core::MemoryProt::CpuExec)) {
         LOG_ERROR(Kernel_Vmm, "Executable permissions are not allowed.");
-        return ORBIS_KERNEL_ERROR_EACCES;
+        return ORBIS_KERNEL_ERROR_EINVAL;
     }
 
     const auto map_flags = static_cast<Core::MemoryMapFlags>(flags);
@@ -537,11 +537,16 @@ s32 PS4_SYSV_ABI sceKernelMemoryPoolCommit(void* addr, u64 len, s32 type, s32 pr
         return ORBIS_KERNEL_ERROR_EINVAL;
     }
 
+    const auto mem_prot = static_cast<Core::MemoryProt>(prot);
+    if (True(mem_prot & Core::MemoryProt::CpuExec)) {
+        LOG_ERROR(Kernel_Vmm, "Executable permissions are not allowed.");
+        return ORBIS_KERNEL_ERROR_EINVAL;
+    }
+
     LOG_INFO(Kernel_Vmm, "addr = {}, len = {:#x}, type = {:#x}, prot = {:#x}, flags = {:#x}",
              fmt::ptr(addr), len, type, prot, flags);
 
     const VAddr in_addr = reinterpret_cast<VAddr>(addr);
-    const auto mem_prot = static_cast<Core::MemoryProt>(prot);
     auto* memory = Core::Memory::Instance();
     return memory->PoolCommit(in_addr, len, mem_prot, type);
 }
@@ -651,13 +656,18 @@ void* PS4_SYSV_ABI posix_mmap(void* addr, u64 len, s32 prot, s32 flags, s32 fd,
     const auto mem_prot = static_cast<Core::MemoryProt>(prot);
     const auto mem_flags = static_cast<Core::MemoryMapFlags>(flags);
 
+    // mmap is less restrictive than other functions in regards to alignment
+    // To avoid potential issues, align address and size here.
+    const VAddr aligned_addr = Common::AlignDown(std::bit_cast<VAddr>(addr), 16_KB);
+    const u64 aligned_size = Common::AlignUp(len, 16_KB);
+
     s32 result = ORBIS_OK;
     if (fd == -1) {
-        result = memory->MapMemory(&addr_out, std::bit_cast<VAddr>(addr), len, mem_prot, mem_flags,
+        result = memory->MapMemory(&addr_out, aligned_addr, aligned_size, mem_prot, mem_flags,
                                    Core::VMAType::Flexible, "anon", false);
     } else {
-        result = memory->MapFile(&addr_out, std::bit_cast<VAddr>(addr), len, mem_prot, mem_flags,
-                                 fd, phys_addr);
+        result = memory->MapFile(&addr_out, aligned_addr, aligned_size, mem_prot, mem_flags, fd,
+                                 phys_addr);
     }
 
     if (result != ORBIS_OK) {

src/core/memory.cpp

@@ -226,8 +226,6 @@ PAddr MemoryManager::Allocate(PAddr search_start, PAddr search_end, u64 size, u6
 void MemoryManager::Free(PAddr phys_addr, u64 size) {
     std::scoped_lock lk{mutex};
 
-    auto dmem_area = CarveDmemArea(phys_addr, size);
-
     // Release any dmem mappings that reference this physical block.
     std::vector<std::pair<VAddr, u64>> remove_list;
     for (const auto& [addr, mapping] : vma_map) {
@@ -235,22 +233,46 @@ void MemoryManager::Free(PAddr phys_addr, u64 size) {
             continue;
         }
         if (mapping.phys_base <= phys_addr && phys_addr < mapping.phys_base + mapping.size) {
-            auto vma_segment_start_addr = phys_addr - mapping.phys_base + addr;
-            LOG_INFO(Kernel_Vmm, "Unmaping direct mapping {:#x} with size {:#x}",
-                     vma_segment_start_addr, size);
+            const auto vma_start_offset = phys_addr - mapping.phys_base;
+            const auto addr_in_vma = mapping.base + vma_start_offset;
+            const auto size_in_vma =
+                mapping.size - vma_start_offset > size ? size : mapping.size - vma_start_offset;
+
+            LOG_INFO(Kernel_Vmm, "Unmaping direct mapping {:#x} with size {:#x}", addr_in_vma,
+                     size_in_vma);
             // Unmaping might erase from vma_map. We can't do it here.
-            remove_list.emplace_back(vma_segment_start_addr, size);
+            remove_list.emplace_back(addr_in_vma, size_in_vma);
         }
     }
     for (const auto& [addr, size] : remove_list) {
         UnmapMemoryImpl(addr, size);
     }
 
-    // Mark region as free and attempt to coalesce it with neighbours.
-    auto& area = dmem_area->second;
-    area.dma_type = DMAType::Free;
-    area.memory_type = 0;
-    MergeAdjacent(dmem_map, dmem_area);
+    // Unmap all dmem areas within this area.
+    auto phys_addr_to_search = phys_addr;
+    auto remaining_size = size;
+    auto dmem_area = FindDmemArea(phys_addr);
+    while (dmem_area != dmem_map.end() && remaining_size > 0) {
+        // Carve a free dmem area in place of this one.
+        const auto start_phys_addr =
+            phys_addr > dmem_area->second.base ? phys_addr : dmem_area->second.base;
+        const auto offset_in_dma = start_phys_addr - dmem_area->second.base;
+        const auto size_in_dma = dmem_area->second.size - offset_in_dma > remaining_size
+                                     ? remaining_size
+                                     : dmem_area->second.size - offset_in_dma;
+        const auto dmem_handle = CarveDmemArea(start_phys_addr, size_in_dma);
+        auto& new_dmem_area = dmem_handle->second;
+        new_dmem_area.dma_type = DMAType::Free;
+        new_dmem_area.memory_type = 0;
+
+        // Merge the new dmem_area with dmem_map
+        MergeAdjacent(dmem_map, dmem_handle);
+
+        // Get the next relevant dmem area.
+        phys_addr_to_search = phys_addr + size_in_dma;
+        remaining_size -= size_in_dma;
+        dmem_area = FindDmemArea(phys_addr_to_search);
+    }
 }
 
 s32 MemoryManager::PoolCommit(VAddr virtual_addr, u64 size, MemoryProt prot, s32 mtype) {
@@ -286,6 +308,11 @@ s32 MemoryManager::PoolCommit(VAddr virtual_addr, u64 size, MemoryProt prot, s32
         pool_budget -= size;
     }
 
+    if (True(prot & MemoryProt::CpuWrite)) {
+        // On PS4, read is appended to write mappings.
+        prot |= MemoryProt::CpuRead;
+    }
+
     // Carve out the new VMA representing this mapping
     const auto new_vma_handle = CarveVMA(mapped_addr, size);
     auto& new_vma = new_vma_handle->second;
@@ -465,8 +492,12 @@ s32 MemoryManager::MapMemory(void** out_addr, VAddr virtual_addr, u64 size, Memo
         MergeAdjacent(fmem_map, new_fmem_handle);
     }
 
-    const bool is_exec = True(prot & MemoryProt::CpuExec);
+    if (True(prot & MemoryProt::CpuWrite)) {
+        // On PS4, read is appended to write mappings.
+        prot |= MemoryProt::CpuRead;
+    }
 
+    const bool is_exec = True(prot & MemoryProt::CpuExec);
     new_vma.disallow_merge = True(flags & MemoryMapFlags::NoCoalesce);
     new_vma.prot = prot;
     new_vma.name = name;
@@ -530,6 +561,11 @@ s32 MemoryManager::MapFile(void** out_addr, VAddr virtual_addr, u64 size, Memory
         return ORBIS_KERNEL_ERROR_EBADF;
     }
 
+    if (True(prot & MemoryProt::CpuWrite)) {
+        // On PS4, read is appended to write mappings.
+        prot |= MemoryProt::CpuRead;
+    }
+
     const auto handle = file->f.GetFileMapping();
 
     impl.MapFile(mapped_addr, size_aligned, phys_addr, std::bit_cast<u32>(prot), handle);
@@ -639,7 +675,7 @@ u64 MemoryManager::UnmapBytesFromEntry(VAddr virtual_addr, VirtualMemoryArea vma
         auto remaining_size = adjusted_size;
         DMemHandle dmem_handle = FindDmemArea(phys_addr);
         while (dmem_handle != dmem_map.end() && remaining_size > 0) {
-            const auto start_in_dma = phys_base - dmem_handle->second.base;
+            const auto start_in_dma = phys_addr - dmem_handle->second.base;
             const auto size_in_dma = dmem_handle->second.size - start_in_dma > remaining_size
                                          ? remaining_size
                                          : dmem_handle->second.size - start_in_dma;
@@ -738,7 +774,8 @@ s32 MemoryManager::QueryProtection(VAddr addr, void** start, void** end, u32* pr
     return ORBIS_OK;
 }
 
-s64 MemoryManager::ProtectBytes(VAddr addr, VirtualMemoryArea vma_base, u64 size, MemoryProt prot) {
+s64 MemoryManager::ProtectBytes(VAddr addr, VirtualMemoryArea& vma_base, u64 size,
+                                MemoryProt prot) {
     const auto start_in_vma = addr - vma_base.base;
     const auto adjusted_size =
         vma_base.size - start_in_vma < size ? vma_base.size - start_in_vma : size;
@@ -748,8 +785,10 @@ s64 MemoryManager::ProtectBytes(VAddr addr, VirtualMemoryArea vma_base, u64 size
         return adjusted_size;
     }
 
-    // Change protection
-    vma_base.prot = prot;
+    if (True(prot & MemoryProt::CpuWrite)) {
+        // On PS4, read is appended to write mappings.
+        prot |= MemoryProt::CpuRead;
+    }
 
     // Set permissions
     Core::MemoryPermission perms{};
@@ -773,6 +812,15 @@ s64 MemoryManager::ProtectBytes(VAddr addr, VirtualMemoryArea vma_base, u64 size
         perms |= Core::MemoryPermission::ReadWrite;
     }
 
+    if (vma_base.type == VMAType::Direct || vma_base.type == VMAType::Pooled) {
+        // On PS4, execute permissions are hidden from direct memory mappings.
+        // Tests show that execute permissions still apply, so handle this after reading perms.
+        prot &= ~MemoryProt::CpuExec;
+    }
+
+    // Change protection
+    vma_base.prot = prot;
+
     impl.Protect(addr, size, perms);
 
     return adjusted_size;
@@ -783,8 +831,8 @@ s32 MemoryManager::Protect(VAddr addr, u64 size, MemoryProt prot) {
 
     // Validate protection flags
     constexpr static MemoryProt valid_flags =
-        MemoryProt::NoAccess | MemoryProt::CpuRead | MemoryProt::CpuReadWrite |
-        MemoryProt::CpuExec | MemoryProt::GpuRead | MemoryProt::GpuWrite | MemoryProt::GpuReadWrite;
+        MemoryProt::NoAccess | MemoryProt::CpuRead | MemoryProt::CpuWrite | MemoryProt::CpuExec |
+        MemoryProt::GpuRead | MemoryProt::GpuWrite | MemoryProt::GpuReadWrite;
 
     MemoryProt invalid_flags = prot & ~valid_flags;
     if (invalid_flags != MemoryProt::NoAccess) {

src/core/memory.h

@@ -30,7 +30,8 @@ namespace Core {
 enum class MemoryProt : u32 {
     NoAccess = 0,
     CpuRead = 1,
-    CpuReadWrite = 2,
+    CpuWrite = 2,
+    CpuReadWrite = 3,
     CpuExec = 4,
     GpuRead = 16,
     GpuWrite = 32,
@@ -239,7 +240,7 @@ public:
 
     s32 Protect(VAddr addr, u64 size, MemoryProt prot);
 
-    s64 ProtectBytes(VAddr addr, VirtualMemoryArea vma_base, u64 size, MemoryProt prot);
+    s64 ProtectBytes(VAddr addr, VirtualMemoryArea& vma_base, u64 size, MemoryProt prot);
 
     s32 VirtualQuery(VAddr addr, s32 flags, ::Libraries::Kernel::OrbisVirtualQueryInfo* info);