From 016f976b1f18517e409dcdd3ef687afeb02bfded Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 30 Aug 2026 22:42:07 +0200 Subject: [PATCH] Fix a batch of missing or wrong range validation Memory::IsValidAddress and friends tested the extended-RAM range with (address & 0x3F000000), i.e. at 16MB granularity, so they accepted the whole 16MB block containing the end of RAM. That's harmless at 32MB and 64MB, but the Sora no Kiseki SC/3rd HD remasters run with 0x04C00000, so addresses from 0x0CC00000 to 0x0CFFFFFF read as valid, and MaxSizeAtAddress then underflowed to ~4GB there - which defeats ClampValidSizeAt and IsValidRange entirely for that window. Mask with 0x3FFFFFFF instead, in all five helpers and the copies in MemMapFunctions.cpp. IsValidTextureAddress's extended-RAM branch repeated the first branch's whole mask rather than just its alignment bits, so it was dead code and extended RAM was never accepted as a texture source. ComputeTextureHash checked IsValidAddress(addr + sizeInRAM), i.e. only the end address, which can land in a different valid region than the start - a VRAM texture with a large enough computed size ends exactly at the base of RAM and "passes" while reading far past the 8MB VRAM view. Use IsValidRange. TextureReplacer::ComputeHash's strided path had no range check at all, unlike the contiguous path right above it. Also clamp the pack-supplied reduce-hash factor to 1.0 - it's a reduction, and the ini parser only rejects exactly 0. ZipExtractFileToMemory read an uninitialized zip_stat when zip_stat_index failed (it ignored the return value) and sized a host allocation directly from the zip's declared uncompressed size. Reached just by opening an archive. Memory::Reinit ignored Init()'s return value, and DoState fed it a memory size taken straight from the savestate. A bogus size made the map fail to allocate and left base null, after which DoMemoryVoid wrote RAM through it. Validate the size, propagate the failure, and roll back to the previous size if reinit fails. 314 pspautotests pass, all unit tests pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2 --- Core/Loaders.cpp | 22 +++++++++++++++--- Core/MemMap.cpp | 38 +++++++++++++++++++++++++------ Core/MemMap.h | 16 +++++++------ Core/MemMapFunctions.cpp | 10 ++++---- GPU/Common/TextureCacheCommon.cpp | 4 +++- GPU/Common/TextureReplacer.cpp | 10 ++++++++ 6 files changed, 77 insertions(+), 23 deletions(-) diff --git a/Core/Loaders.cpp b/Core/Loaders.cpp index 0d5b138598..0c85f8975c 100644 --- a/Core/Loaders.cpp +++ b/Core/Loaders.cpp @@ -427,15 +427,31 @@ inline char asciitolower(char in) { return in; } +// Only used for small metadata files (PARAM.SFO, plugin ini). The size below is far above +// anything legitimate - it exists because the size comes from the zip's central directory, +// i.e. straight from an untrusted file. +static const u64 MAX_ZIP_EXTRACT_TO_MEMORY_SIZE = 16 * 1024 * 1024; + static bool ZipExtractFileToMemory(struct zip *z, int fileIndex, std::string *data) { - struct zip_stat zstat; - zip_stat_index(z, fileIndex, 0, &zstat); + zip_stat_t zstat{}; + if (zip_stat_index(z, fileIndex, 0, &zstat) != 0) { + ERROR_LOG(Log::HLE, "zip_stat_index failed for file %d in zip", fileIndex); + return false; + } + if (!(zstat.valid & ZIP_STAT_SIZE)) { + ERROR_LOG(Log::HLE, "No size for file %d in zip", fileIndex); + return false; + } if (zstat.size == 0) { data->clear(); return true; } + if (zstat.size > MAX_ZIP_EXTRACT_TO_MEMORY_SIZE) { + ERROR_LOG(Log::HLE, "Refusing to extract file %d from zip: declared size %llu is implausible", fileIndex, (unsigned long long)zstat.size); + return false; + } - size_t readSize = zstat.size; + size_t readSize = (size_t)zstat.size; data->resize(readSize); zip_file *zf = zip_fopen_index(z, fileIndex, 0); diff --git a/Core/MemMap.cpp b/Core/MemMap.cpp index 06d9596569..b27fcec5b3 100644 --- a/Core/MemMap.cpp +++ b/Core/MemMap.cpp @@ -315,9 +315,16 @@ void MemoryMap_Shutdown() { #endif } +// On some 32 bit platforms (like old Android, old iOS, etc.), there are/were restrictions on memory map sizes. +// This particular size I can't find any sources for though. +static const int MAX_MMAP_SIZE = 31 * 1024 * 1024; + +// Every size we actually use (32MB, 64MB, and the 76MB remasters) is a whole number of megabytes. +static bool IsPlausibleMemorySize(u32 size) { + return size != 0 && size <= (u32)MAX_MMAP_SIZE * 3 && (size & 0xFFFFF) == 0; +} + bool Init(MemMapSetupFlags flags) { - // On some 32 bit platforms (like Android, iOS, etc.), you can only map < 32 megs at a time. - const static int MAX_MMAP_SIZE = 31 * 1024 * 1024; _dbg_assert_msg_(g_MemorySize <= MAX_MMAP_SIZE * 3, "ACK - too much memory for three mmap views."); for (size_t i = 0; i < ARRAY_SIZE(views); i++) { if (views[i].flags & MV_IS_PRIMARY_RAM) @@ -339,7 +346,9 @@ bool Init(MemMapSetupFlags flags) { return true; } -void Reinit() { +// Returns false if the new map couldn't be set up - in which case there is no memory map at all, +// and base is null. Callers must not carry on writing to guest memory. +bool Reinit() { _assert_msg_(PSP_GetBootState() == BootState::Complete, "Cannot reinit during startup/shutdown"); Core_NotifyLifecycle(CoreLifecycle::MEMORY_REINITING); // Held across both halves: between Shutdown() and Init() there is no memory map at all, and a @@ -347,8 +356,9 @@ void Reinit() { CoreShutdownLock coreLock = Core_LockAgainstShutdown(); MemMapSetupFlags flags = g_setupFlags; Shutdown(); - Init(flags); + const bool success = Init(flags); Core_NotifyLifecycle(CoreLifecycle::MEMORY_REINITED); + return success; } static void DoMemoryVoid(PointerWrap &p, uint32_t start, uint32_t size) { @@ -397,8 +407,10 @@ void DoState(PointerWrap &p) { p.DoMarker("PSPModel"); if (!g_RemasterMode) { g_MemorySize = g_PSPModel == PSP_MODEL_FAT ? RAM_NORMAL_SIZE : RAM_DOUBLE_SIZE; - if (oldMemorySize < g_MemorySize) { - Reinit(); + if (oldMemorySize < g_MemorySize && !Reinit()) { + ERROR_LOG(Log::MemMap, "Failed to reinit memory to %08x bytes", g_MemorySize); + p.SetError(PointerWrap::ERROR_FAILURE); + return; } } } else { @@ -408,8 +420,20 @@ void DoState(PointerWrap &p) { Do(p, g_PSPModel); p.DoMarker("PSPModel"); Do(p, g_MemorySize); - if (oldMemorySize != g_MemorySize) { + if (p.mode == PointerWrap::MODE_READ && !IsPlausibleMemorySize(g_MemorySize)) { + // Straight out of the file, so don't hand it to Init() - a bogus size makes the map + // fail to allocate, and we'd carry on writing RAM through a null base. + ERROR_LOG(Log::MemMap, "Savestate specifies an implausible memory size: %08x", g_MemorySize); + g_MemorySize = oldMemorySize; + p.SetError(PointerWrap::ERROR_FAILURE); + return; + } + if (oldMemorySize != g_MemorySize && !Reinit()) { + ERROR_LOG(Log::MemMap, "Failed to reinit memory to %08x bytes, restoring %08x", g_MemorySize, oldMemorySize); + g_MemorySize = oldMemorySize; Reinit(); + p.SetError(PointerWrap::ERROR_FAILURE); + return; } } diff --git a/Core/MemMap.h b/Core/MemMap.h index 137629d2f7..4205c48323 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -319,7 +319,7 @@ inline bool IsValidAddress(const u32 address) { return true; // 0xBxx above: Let's disallow kernel-flagged VRAM. We don't have it mapped and I am not sure if it's accessible. } else if ((address & 0x3FFFC000) == 0x00010000) { return true; - } else if ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize) { + } else if ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize) { return true; } else { return false; @@ -333,7 +333,7 @@ inline bool IsValid2AlignedAddress(const u32 address) { return true; // 0xBxx above: Let's disallow kernel-flagged VRAM. We don't have it mapped and I am not sure if it's accessible. } else if ((address & 0x3FFFC001) == 0x00010000) { return true; - } else if ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize) { + } else if ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize) { return (address & 1) == 0; } else { return false; @@ -347,7 +347,7 @@ inline bool IsValid4AlignedAddress(const u32 address) { return true; // 0xBxx above: Let's disallow kernel-flagged VRAM. We don't have it mapped and I am not sure if it's accessible. } else if ((address & 0x3FFFC003) == 0x00010000) { return true; - } else if ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize) { + } else if ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize) { return (address & 3) == 0; } else { return false; @@ -362,7 +362,7 @@ inline bool IsValidNAlignedAddress(const u32 address) { return true; // 0xBxx above: Let's disallow kernel-flagged VRAM. We don't have it mapped and I am not sure if it's accessible. } else if ((address & (0x3FFFC000 | (A - 1))) == 0x00010000) { return true; - } else if ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize) { + } else if ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize) { return (address & (A - 1)) == 0; } else { return false; @@ -376,7 +376,7 @@ inline u32 MaxSizeAtAddress(const u32 address) { return 0x04800000 - (address & 0x3FFFFFFF); // VRAM. Same 0xBxx trick as above, modified for this use case. } else if ((address & 0x3FFFC000) == 0x00010000) { return 0x00014000 - (address & 0x3FFFFFFF); - } else if ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize) { + } else if ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize) { return 0x08000000 + g_MemorySize - (address & 0x3FFFFFFF); } else { return 0; @@ -393,8 +393,10 @@ inline bool IsValidTextureAddress(const u32 address) { return true; // Can texture from RAM (not sure if kernel RAM too, but let's allow it). } else if ((address & 0xBF80000F) == 0x04000000) { return true; // 0xBxx above: Let's disallow kernel-flagged VRAM. We don't have it mapped and I am not sure if it's accessible. - } else if ((address & 0x3E00000F) == 0x08000000 && (address & 0x3F000000) >= 0x08000000 && ((address & 0x3F000000) < 0x08000000 + g_MemorySize)) { - return true; // Extended RAM. + } else if ((address & 0x0F) == 0 && (address & 0x3FFFFFFF) >= 0x08000000 && ((address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize)) { + // Extended RAM. This used to repeat the first branch's full mask, which made it dead code - + // only the 16-byte alignment part of it belongs here. + return true; } else if (IsPPGEAtlasFakeAddress(address, nullptr)) { return true; // PPGe atlas texture } else { diff --git a/Core/MemMapFunctions.cpp b/Core/MemMapFunctions.cpp index ad10fe7e35..af5419b1e3 100644 --- a/Core/MemMapFunctions.cpp +++ b/Core/MemMapFunctions.cpp @@ -31,7 +31,7 @@ u8 *GetPointerWriteOrException(const u32 address) { if ((address & 0x3E000000) == 0x08000000 || // RAM (address & 0xBF800000) == 0x04000000 || // VRAM (address & 0x3FFFC000) == 0x00010000 || // Scratchpad - ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize)) { // More RAM (remasters, etc.) + ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize)) { // More RAM (remasters, etc.) return GetPointerWriteUnchecked(address); } else { // Size is not known, we pass 0 to signal that. @@ -44,7 +44,7 @@ const u8 *GetPointerOrException(const u32 address) { if ((address & 0x3E000000) == 0x08000000 || // RAM (address & 0xBF800000) == 0x04000000 || // VRAM (address & 0x3FFFC000) == 0x00010000 || // Scratchpad - ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize)) { // More RAM (remasters, etc.) + ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize)) { // More RAM (remasters, etc.) return GetPointerUnchecked(address); } else { // Size is not known, we pass 0 to signal that. @@ -90,7 +90,7 @@ inline void ReadMemoryOrException(T &var, const u32 address) { if ((address & 0x3E000000) == 0x08000000 || // RAM (address & 0xBF800000) == 0x04000000 || // VRAM (address & 0x3FFFC000) == 0x00010000 || // Scratchpad - ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize)) { // More RAM (remasters, etc.) + ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize)) { // More RAM (remasters, etc.) var = *((const T*)GetPointerUnchecked(address)); } else { Core_MemoryException(address, sizeof(T), currentMIPS->pc, MemoryExceptionType::READ_WORD); @@ -103,7 +103,7 @@ inline void WriteMemoryOrException(u32 address, const T data) { if ((address & 0x3E000000) == 0x08000000 || // RAM (address & 0xBF800000) == 0x04000000 || // VRAM (address & 0x3FFFC000) == 0x00010000 || // Scratchpad - ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize)) { // More RAM (remasters, etc.) + ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize)) { // More RAM (remasters, etc.) *(T*)GetPointerUnchecked(address) = data; } else { Core_MemoryException(address, sizeof(T), currentMIPS->pc, MemoryExceptionType::WRITE_WORD); @@ -113,7 +113,7 @@ inline void WriteMemoryOrException(u32 address, const T data) { bool IsRAMAddress(const u32 address) { if ((address & 0x3E000000) == 0x08000000) { return true; - } else if ((address & 0x3F000000) >= 0x08000000 && (address & 0x3F000000) < 0x08000000 + g_MemorySize) { + } else if ((address & 0x3FFFFFFF) >= 0x08000000 && (address & 0x3FFFFFFF) < 0x08000000 + g_MemorySize) { return true; } else { return false; diff --git a/GPU/Common/TextureCacheCommon.cpp b/GPU/Common/TextureCacheCommon.cpp index 7fdb3cd1f1..1412e90d93 100644 --- a/GPU/Common/TextureCacheCommon.cpp +++ b/GPU/Common/TextureCacheCommon.cpp @@ -410,7 +410,9 @@ static u32 ComputeTextureHash(TextureReplacer &replacer, u32 addr, int bufw, int const u32 *checkp = (const u32 *)Memory::GetPointerOrException(addr); // NOTE: I'm not sure we want to align-check the end, so can't use IsValidTextureAddress here. - if (Memory::IsValidAddress(addr + sizeInRAM)) { + // IsValidAddress on the end address alone isn't enough - the end can land in a different valid + // region than the start, e.g. a VRAM texture whose computed end reaches the base of RAM. + if (Memory::IsValidRange(addr, sizeInRAM)) { gpuStats.perFrame.numTextureDataBytesHashed += sizeInRAM; // return XXH64(checkp, sizeInRAM, 0xBACD7814); diff --git a/GPU/Common/TextureReplacer.cpp b/GPU/Common/TextureReplacer.cpp index 213a171caa..c170ae96d1 100644 --- a/GPU/Common/TextureReplacer.cpp +++ b/GPU/Common/TextureReplacer.cpp @@ -550,6 +550,8 @@ u32 TextureReplacer::ComputeHash(u32 addr, int bufw, int w, int h, bool swizzled reduceHashSize = LookupReduceHashRange(w, h); // default to reduceHashGlobalValue which default is 0.5 } + // It's a reduction factor and comes from the pack's ini, so don't let it hash more than the texture. + reduceHashSize = std::clamp(reduceHashSize, 0.0f, 1.0f); if (bufw <= w) { // We can assume the data is contiguous. These are the total used pixels. @@ -586,6 +588,14 @@ u32 TextureReplacer::ComputeHash(u32 addr, int bufw, int w, int h, bool swizzled const u32 bytesPerLine = (textureBitsPerPixel[fmt] * w) / 8 * reduceHashSize; const u32 stride = (textureBitsPerPixel[fmt] * bufw) / 8; + // Same sanity check as the contiguous path above - the rows are strided, so this is the + // span we actually touch. Without it, a large h walks far past the end of the region. + const u32 sizeInRAM = h > 0 ? (h - 1) * stride + bytesPerLine : 0; + if (Memory::MaxSizeAtAddress(addr) < sizeInRAM) { + ERROR_LOG(Log::G3D, "Can't hash a %d bytes texture at %08x - end point is outside memory", sizeInRAM, addr); + return 0; + } + u32 result = 0; switch (textureHash_) { case ReplacedTextureHash::QUICK: