From 853cf4f8181dd0162e81ebcd61331d8765d089ca Mon Sep 17 00:00:00 2001 From: "Admiral H. Curtiss" Date: Mon, 18 Apr 2022 03:41:14 +0200 Subject: [PATCH 1/2] Common/PointerWrap: Hide internals. --- Source/Core/Common/ChunkFile.h | 1 + Source/Core/Core/IOS/Network/Socket.cpp | 4 ++-- Source/Core/VideoCommon/CPMemory.cpp | 2 +- Source/Core/VideoCommon/Fifo.cpp | 2 +- Source/Core/VideoCommon/FreeLookCamera.cpp | 2 +- 5 files changed, 6 insertions(+), 5 deletions(-) diff --git a/Source/Core/Common/ChunkFile.h b/Source/Core/Common/ChunkFile.h index 867ad199f1..e7791839db 100644 --- a/Source/Core/Common/ChunkFile.h +++ b/Source/Core/Common/ChunkFile.h @@ -46,6 +46,7 @@ public: MODE_VERIFY, // compare }; +private: u8** ptr; Mode mode; diff --git a/Source/Core/Core/IOS/Network/Socket.cpp b/Source/Core/Core/IOS/Network/Socket.cpp index ae5f882e90..20220c2968 100644 --- a/Source/Core/Core/IOS/Network/Socket.cpp +++ b/Source/Core/Core/IOS/Network/Socket.cpp @@ -994,8 +994,8 @@ void WiiSockMan::Convert(sockaddr_in const& from, WiiSockAddrIn& to, s32 addrlen void WiiSockMan::DoState(PointerWrap& p) { - bool saving = - p.mode == PointerWrap::Mode::MODE_WRITE || p.mode == PointerWrap::Mode::MODE_MEASURE; + bool saving = p.GetMode() == PointerWrap::Mode::MODE_WRITE || + p.GetMode() == PointerWrap::Mode::MODE_MEASURE; auto size = pending_polls.size(); p.Do(size); if (!saving) diff --git a/Source/Core/VideoCommon/CPMemory.cpp b/Source/Core/VideoCommon/CPMemory.cpp index 0df0d9f1d8..3a2ee54bc6 100644 --- a/Source/Core/VideoCommon/CPMemory.cpp +++ b/Source/Core/VideoCommon/CPMemory.cpp @@ -26,7 +26,7 @@ void DoCPState(PointerWrap& p) p.Do(g_main_cp_state.vtx_desc); p.DoArray(g_main_cp_state.vtx_attr); p.DoMarker("CP Memory"); - if (p.mode == PointerWrap::MODE_READ) + if (p.GetMode() == PointerWrap::MODE_READ) { CopyPreprocessCPStateFromMain(); VertexLoaderManager::g_bases_dirty = true; diff --git a/Source/Core/VideoCommon/Fifo.cpp b/Source/Core/VideoCommon/Fifo.cpp index ccee18f40e..18ef1b432f 100644 --- a/Source/Core/VideoCommon/Fifo.cpp +++ b/Source/Core/VideoCommon/Fifo.cpp @@ -94,7 +94,7 @@ void DoState(PointerWrap& p) p.DoPointer(write_ptr, s_video_buffer); s_video_buffer_write_ptr = write_ptr; p.DoPointer(s_video_buffer_read_ptr, s_video_buffer); - if (p.mode == PointerWrap::MODE_READ && s_use_deterministic_gpu_thread) + if (p.GetMode() == PointerWrap::MODE_READ && s_use_deterministic_gpu_thread) { // We're good and paused, right? s_video_buffer_seen_ptr = s_video_buffer_pp_read_ptr = s_video_buffer_read_ptr; diff --git a/Source/Core/VideoCommon/FreeLookCamera.cpp b/Source/Core/VideoCommon/FreeLookCamera.cpp index 55d86d9d27..34677acbb8 100644 --- a/Source/Core/VideoCommon/FreeLookCamera.cpp +++ b/Source/Core/VideoCommon/FreeLookCamera.cpp @@ -298,7 +298,7 @@ Common::Vec2 FreeLookCamera::GetFieldOfViewMultiplier() const void FreeLookCamera::DoState(PointerWrap& p) { - if (p.mode == PointerWrap::MODE_WRITE || p.mode == PointerWrap::MODE_MEASURE) + if (p.GetMode() == PointerWrap::MODE_WRITE || p.GetMode() == PointerWrap::MODE_MEASURE) { p.Do(m_current_type); if (m_camera_controller) From ef760ee012a1a7a9d8997fecce4f0c9a7122e1bd Mon Sep 17 00:00:00 2001 From: "Admiral H. Curtiss" Date: Mon, 18 Apr 2022 04:13:25 +0200 Subject: [PATCH 2/2] Common/PointerWrap: Prevent reads/writes past the end of the buffer. --- Source/Core/Common/ChunkFile.h | 34 ++++++++++++++------ Source/Core/Core/State.cpp | 28 ++++++++-------- Source/Core/UICommon/GameFileCache.cpp | 10 +++--- Source/Core/VideoCommon/TextureCacheBase.cpp | 6 ++++ 4 files changed, 51 insertions(+), 27 deletions(-) diff --git a/Source/Core/Common/ChunkFile.h b/Source/Core/Common/ChunkFile.h index e7791839db..9fd785459e 100644 --- a/Source/Core/Common/ChunkFile.h +++ b/Source/Core/Common/ChunkFile.h @@ -47,11 +47,16 @@ public: }; private: - u8** ptr; + u8** m_ptr_current; + u8* m_ptr_end; Mode mode; public: - PointerWrap(u8** ptr_, Mode mode_) : ptr(ptr_), mode(mode_) {} + PointerWrap(u8** ptr, size_t size, Mode mode_) + : m_ptr_current(ptr), m_ptr_end(*ptr + size), mode(mode_) + { + } + void SetMode(Mode mode_) { mode = mode_; } Mode GetMode() const { return mode; } template @@ -209,8 +214,13 @@ public: [[nodiscard]] u8* DoExternal(u32& count) { Do(count); - u8* current = *ptr; - *ptr += count; + u8* current = *m_ptr_current; + *m_ptr_current += count; + if (mode != MODE_MEASURE && *m_ptr_current > m_ptr_end) + { + // trying to read/write past the end of the buffer, prevent this + mode = MODE_MEASURE; + } return current; } @@ -320,26 +330,32 @@ private: DOLPHIN_FORCE_INLINE void DoVoid(void* data, u32 size) { + if (mode != MODE_MEASURE && (*m_ptr_current + size) > m_ptr_end) + { + // trying to read/write past the end of the buffer, prevent this + mode = MODE_MEASURE; + } + switch (mode) { case MODE_READ: - memcpy(data, *ptr, size); + memcpy(data, *m_ptr_current, size); break; case MODE_WRITE: - memcpy(*ptr, data, size); + memcpy(*m_ptr_current, data, size); break; case MODE_MEASURE: break; case MODE_VERIFY: - DEBUG_ASSERT_MSG(COMMON, !memcmp(data, *ptr, size), + DEBUG_ASSERT_MSG(COMMON, !memcmp(data, *m_ptr_current, size), "Savestate verification failure: buf {} != {} (size {}).\n", fmt::ptr(data), - fmt::ptr(*ptr), size); + fmt::ptr(*m_ptr_current), size); break; } - *ptr += size; + *m_ptr_current += size; } }; diff --git a/Source/Core/Core/State.cpp b/Source/Core/Core/State.cpp index b1a3c70910..71e8642f06 100644 --- a/Source/Core/Core/State.cpp +++ b/Source/Core/Core/State.cpp @@ -225,8 +225,8 @@ void LoadFromBuffer(std::vector& buffer) Core::RunOnCPUThread( [&] { - u8* ptr = &buffer[0]; - PointerWrap p(&ptr, PointerWrap::MODE_READ); + u8* ptr = buffer.data(); + PointerWrap p(&ptr, buffer.size(), PointerWrap::MODE_READ); DoState(p); }, true); @@ -237,14 +237,14 @@ void SaveToBuffer(std::vector& buffer) Core::RunOnCPUThread( [&] { u8* ptr = nullptr; - PointerWrap p(&ptr, PointerWrap::MODE_MEASURE); + PointerWrap p_measure(&ptr, 0, PointerWrap::MODE_MEASURE); - DoState(p); + DoState(p_measure); const size_t buffer_size = reinterpret_cast(ptr); buffer.resize(buffer_size); - ptr = &buffer[0]; - p.SetMode(PointerWrap::MODE_WRITE); + ptr = buffer.data(); + PointerWrap p(&ptr, buffer_size, PointerWrap::MODE_WRITE); DoState(p); }, true); @@ -412,20 +412,22 @@ void SaveAs(const std::string& filename, bool wait) [&] { // Measure the size of the buffer. u8* ptr = nullptr; - PointerWrap p(&ptr, PointerWrap::MODE_MEASURE); - DoState(p); + PointerWrap p_measure(&ptr, 0, PointerWrap::MODE_MEASURE); + DoState(p_measure); const size_t buffer_size = reinterpret_cast(ptr); // Then actually do the write. + PointerWrap::Mode p_mode; { std::lock_guard lk(g_cs_current_buffer); g_current_buffer.resize(buffer_size); - ptr = &g_current_buffer[0]; - p.SetMode(PointerWrap::MODE_WRITE); + ptr = g_current_buffer.data(); + PointerWrap p(&ptr, buffer_size, PointerWrap::MODE_WRITE); DoState(p); + p_mode = p.GetMode(); } - if (p.GetMode() == PointerWrap::MODE_WRITE) + if (p_mode == PointerWrap::MODE_WRITE) { Core::DisplayMessage("Saving State...", 1000); @@ -588,8 +590,8 @@ void LoadAs(const std::string& filename) if (!buffer.empty()) { - u8* ptr = &buffer[0]; - PointerWrap p(&ptr, PointerWrap::MODE_READ); + u8* ptr = buffer.data(); + PointerWrap p(&ptr, buffer.size(), PointerWrap::MODE_READ); DoState(p); loaded = true; loadedSuccessfully = (p.GetMode() == PointerWrap::MODE_READ); diff --git a/Source/Core/UICommon/GameFileCache.cpp b/Source/Core/UICommon/GameFileCache.cpp index 1040c24b2f..a41c07553c 100644 --- a/Source/Core/UICommon/GameFileCache.cpp +++ b/Source/Core/UICommon/GameFileCache.cpp @@ -230,14 +230,14 @@ bool GameFileCache::SyncCacheFile(bool save) { // Measure the size of the buffer. u8* ptr = nullptr; - PointerWrap p(&ptr, PointerWrap::MODE_MEASURE); - DoState(&p); + PointerWrap p_measure(&ptr, 0, PointerWrap::MODE_MEASURE); + DoState(&p_measure); const size_t buffer_size = reinterpret_cast(ptr); // Then actually do the write. std::vector buffer(buffer_size); - ptr = &buffer[0]; - p.SetMode(PointerWrap::MODE_WRITE); + ptr = buffer.data(); + PointerWrap p(&ptr, buffer_size, PointerWrap::MODE_WRITE); DoState(&p, buffer_size); if (f.WriteBytes(buffer.data(), buffer.size())) success = true; @@ -248,7 +248,7 @@ bool GameFileCache::SyncCacheFile(bool save) if (!buffer.empty() && f.ReadBytes(buffer.data(), buffer.size())) { u8* ptr = buffer.data(); - PointerWrap p(&ptr, PointerWrap::MODE_READ); + PointerWrap p(&ptr, buffer.size(), PointerWrap::MODE_READ); DoState(&p, buffer.size()); if (p.GetMode() == PointerWrap::MODE_READ) success = true; diff --git a/Source/Core/VideoCommon/TextureCacheBase.cpp b/Source/Core/VideoCommon/TextureCacheBase.cpp index 078abf10b1..b1c9ead97a 100644 --- a/Source/Core/VideoCommon/TextureCacheBase.cpp +++ b/Source/Core/VideoCommon/TextureCacheBase.cpp @@ -459,6 +459,12 @@ void TextureCacheBase::SerializeTexture(AbstractTexture* tex, const TextureConfi // needing to allocate/free an extra buffer. u8* texture_data = p.DoExternal(total_size); + if (p.GetMode() == PointerWrap::MODE_MEASURE) + { + ERROR_LOG_FMT(VIDEO, "Couldn't acquire {} bytes for serializing texture.", total_size); + return; + } + if (!skip_readback) { // Save out each layer of the texture to the pointer.