From 5c9bb80638ec05b32eaa129a8c763ac6bb3a5cb4 Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sat, 13 Apr 2024 12:08:43 +0200 Subject: [PATCH] Memmap: Replace GetPointer with GetSpanForAddress To ensure memory safety, callers of GetPointer have to perform a bounds check. But how is this bounds check supposed to be performed? GetPointerForRange contained one implementation of a bounds check, but it was cumbersome, and it also isn't obvious why it's correct. To make doing the right thing easier, this commit changes GetPointer to return a span that tells the caller how many bytes it's allowed to access. --- Source/Core/Core/HW/Memmap.cpp | 35 +++++++++++-------- Source/Core/Core/HW/Memmap.h | 13 +++++-- .../VideoBackends/Software/TextureSampler.cpp | 5 ++- Source/Core/VideoCommon/TextureInfo.cpp | 8 +++-- .../Core/VideoCommon/VertexLoaderManager.cpp | 17 ++++++--- 5 files changed, 53 insertions(+), 25 deletions(-) diff --git a/Source/Core/Core/HW/Memmap.cpp b/Source/Core/Core/HW/Memmap.cpp index f063665200..be58fbfc69 100644 --- a/Source/Core/Core/HW/Memmap.cpp +++ b/Source/Core/Core/HW/Memmap.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include "Common/ChunkFile.h" @@ -400,22 +401,23 @@ void MemoryManager::Clear() u8* MemoryManager::GetPointerForRange(u32 address, size_t size) const { - // Make sure we don't have a range spanning 2 separate banks - if (size >= GetExRamSizeReal()) + std::span span = GetSpanForAddress(address); + + if (span.data() == nullptr) { + // The address isn't in a valid memory region. + // A panic alert has already been raised by GetPointer, so let's not raise another one. + return nullptr; + } + + if (span.size() < size) + { + // The start address is in a valid region, but the end address is beyond the end of that region. PanicAlertFmt("Oversized range in GetPointerForRange. {:x} bytes at {:#010x}", size, address); return nullptr; } - // Check that the beginning and end of the range are valid - u8* pointer = GetPointer(address); - if (pointer == nullptr || (size != 0 && GetPointer(address + u32(size) - 1) == nullptr)) - { - // A panic alert has already been raised by GetPointer - return nullptr; - } - - return pointer; + return span.data(); } void MemoryManager::CopyFromEmu(void* data, u32 address, size_t size) const @@ -487,24 +489,27 @@ std::string MemoryManager::GetString(u32 em_address, size_t size) } } -u8* MemoryManager::GetPointer(u32 address) const +std::span MemoryManager::GetSpanForAddress(u32 address) const { // TODO: Should we be masking off more bits here? Can all devices access // EXRAM? address &= 0x3FFFFFFF; if (address < GetRamSizeReal()) - return m_ram + address; + return std::span(m_ram + address, GetRamSizeReal() - address); if (m_exram) { if ((address >> 28) == 0x1 && (address & 0x0fffffff) < GetExRamSizeReal()) - return m_exram + (address & GetExRamMask()); + { + return std::span(m_exram + (address & GetExRamMask()), + GetExRamSizeReal() - (address & GetExRamMask())); + } } auto& ppc_state = m_system.GetPPCState(); PanicAlertFmt("Unknown Pointer {:#010x} PC {:#010x} LR {:#010x}", address, ppc_state.pc, LR(ppc_state)); - return nullptr; + return {}; } u8 MemoryManager::Read_U8(u32 address) const diff --git a/Source/Core/Core/HW/Memmap.h b/Source/Core/Core/HW/Memmap.h index e90e641f55..e0708605db 100644 --- a/Source/Core/Core/HW/Memmap.h +++ b/Source/Core/Core/HW/Memmap.h @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -105,10 +106,16 @@ public: // Routines to access physically addressed memory, designed for use by // emulated hardware outside the CPU. Use "Device_" prefix. std::string GetString(u32 em_address, size_t size = 0); - // WARNING: Incrementing the pointer returned by GetPointer is unsafe without additional bounds - // checks. New code should use other functions instead, like GetPointerForRange or CopyFromEmu. - u8* GetPointer(u32 address) const; + + // If the specified guest address is within a valid memory region, returns a span starting at the + // host address corresponding to the specified address and ending where the memory region ends. + // Otherwise, returns a 0-length span starting at nullptr. + std::span GetSpanForAddress(u32 address) const; + + // If the specified range is within a single valid memory region, returns a pointer to the start + // of the corresponding range in host memory. Otherwise, returns nullptr. u8* GetPointerForRange(u32 address, size_t size) const; + void CopyFromEmu(void* data, u32 address, size_t size) const; void CopyToEmu(u32 address, const void* data, size_t size); void Memset(u32 address, u8 value, size_t size); diff --git a/Source/Core/VideoBackends/Software/TextureSampler.cpp b/Source/Core/VideoBackends/Software/TextureSampler.cpp index d5222384f0..f9182441bb 100644 --- a/Source/Core/VideoBackends/Software/TextureSampler.cpp +++ b/Source/Core/VideoBackends/Software/TextureSampler.cpp @@ -5,6 +5,7 @@ #include #include +#include #include "Common/CommonTypes.h" #include "Common/MsgHandler.h" @@ -137,7 +138,9 @@ void SampleMip(s32 s, s32 t, s32 mip, bool linear, u8 texmap, u8* sample) auto& memory = system.GetMemory(); const u32 imageBase = texUnit.texImage3.image_base << 5; - imageSrc = memory.GetPointer(imageBase); + // TODO: For memory safety, we need to check the size of this span + std::span span = memory.GetSpanForAddress(imageBase); + imageSrc = span.data(); } int image_width_minus_1 = ti0.width; diff --git a/Source/Core/VideoCommon/TextureInfo.cpp b/Source/Core/VideoCommon/TextureInfo.cpp index b73461ba33..a1bccb0ebe 100644 --- a/Source/Core/VideoCommon/TextureInfo.cpp +++ b/Source/Core/VideoCommon/TextureInfo.cpp @@ -3,6 +3,8 @@ #include "VideoCommon/TextureInfo.h" +#include + #include #include @@ -47,8 +49,10 @@ TextureInfo TextureInfo::FromStage(u32 stage) auto& system = Core::System::GetInstance(); auto& memory = system.GetMemory(); - return TextureInfo(stage, memory.GetPointer(address), tlut_ptr, address, texture_format, - tlut_format, width, height, false, nullptr, nullptr, mip_count); + // TODO: For memory safety, we need to check the size of this span + std::span span = memory.GetSpanForAddress(address); + return TextureInfo(stage, span.data(), tlut_ptr, address, texture_format, tlut_format, width, + height, false, nullptr, nullptr, mip_count); } TextureInfo::TextureInfo(u32 stage, const u8* ptr, const u8* tlut_ptr, u32 address, diff --git a/Source/Core/VideoCommon/VertexLoaderManager.cpp b/Source/Core/VideoCommon/VertexLoaderManager.cpp index 8ae9a9aa46..0c48bf32a8 100644 --- a/Source/Core/VideoCommon/VertexLoaderManager.cpp +++ b/Source/Core/VideoCommon/VertexLoaderManager.cpp @@ -91,26 +91,35 @@ void UpdateVertexArrayPointers() // Note: Only array bases 0 through 11 are used by the Vertex loaders. // 12 through 15 are used for loading data into xfmem. // We also only update the array base if the vertex description states we are going to use it. + // TODO: For memory safety, we need to check the sizes returned by GetSpanForAddress if (IsIndexed(g_main_cp_state.vtx_desc.low.Position)) + { cached_arraybases[CPArray::Position] = - memory.GetPointer(g_main_cp_state.array_bases[CPArray::Position]); + memory.GetSpanForAddress(g_main_cp_state.array_bases[CPArray::Position]).data(); + } if (IsIndexed(g_main_cp_state.vtx_desc.low.Normal)) + { cached_arraybases[CPArray::Normal] = - memory.GetPointer(g_main_cp_state.array_bases[CPArray::Normal]); + memory.GetSpanForAddress(g_main_cp_state.array_bases[CPArray::Normal]).data(); + } for (u8 i = 0; i < g_main_cp_state.vtx_desc.low.Color.Size(); i++) { if (IsIndexed(g_main_cp_state.vtx_desc.low.Color[i])) + { cached_arraybases[CPArray::Color0 + i] = - memory.GetPointer(g_main_cp_state.array_bases[CPArray::Color0 + i]); + memory.GetSpanForAddress(g_main_cp_state.array_bases[CPArray::Color0 + i]).data(); + } } for (u8 i = 0; i < g_main_cp_state.vtx_desc.high.TexCoord.Size(); i++) { if (IsIndexed(g_main_cp_state.vtx_desc.high.TexCoord[i])) + { cached_arraybases[CPArray::TexCoord0 + i] = - memory.GetPointer(g_main_cp_state.array_bases[CPArray::TexCoord0 + i]); + memory.GetSpanForAddress(g_main_cp_state.array_bases[CPArray::TexCoord0 + i]).data(); + } } g_bases_dirty = false;