From d1441684420dc43edea6b07225d6ac79a13d07f4 Mon Sep 17 00:00:00 2001 From: Liam Date: Fri, 14 Jul 2023 22:19:59 -0400 Subject: [PATCH 1/2] memory: minimize dependency on process --- src/core/memory.cpp | 115 ++++++++++++++++++++++---------------------- src/core/memory.h | 13 +++-- 2 files changed, 63 insertions(+), 65 deletions(-) diff --git a/src/core/memory.cpp b/src/core/memory.cpp index e1fbe8e00a..179685b722 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -183,13 +183,13 @@ struct Memory::Impl { return string; } - void WalkBlock(const Kernel::KProcess& process, const Common::ProcessAddress addr, - const std::size_t size, auto on_unmapped, auto on_memory, auto on_rasterizer, - auto increment) { - const auto& page_table = process.GetPageTable().PageTableImpl(); + bool WalkBlock(const Common::ProcessAddress addr, const std::size_t size, auto on_unmapped, + auto on_memory, auto on_rasterizer, auto increment) { + const auto& page_table = system.ApplicationProcess()->GetPageTable().PageTableImpl(); std::size_t remaining_size = size; std::size_t page_index = addr >> YUZU_PAGEBITS; std::size_t page_offset = addr & YUZU_PAGEMASK; + bool user_accessible = true; while (remaining_size) { const std::size_t copy_amount = @@ -200,6 +200,7 @@ struct Memory::Impl { const auto [pointer, type] = page_table.pointers[page_index].PointerType(); switch (type) { case Common::PageType::Unmapped: { + user_accessible = false; on_unmapped(copy_amount, current_vaddr); break; } @@ -227,13 +228,15 @@ struct Memory::Impl { increment(copy_amount); remaining_size -= copy_amount; } + + return user_accessible; } template - void ReadBlockImpl(const Kernel::KProcess& process, const Common::ProcessAddress src_addr, - void* dest_buffer, const std::size_t size) { - WalkBlock( - process, src_addr, size, + bool ReadBlockImpl(const Common::ProcessAddress src_addr, void* dest_buffer, + const std::size_t size) { + return WalkBlock( + src_addr, size, [src_addr, size, &dest_buffer](const std::size_t copy_amount, const Common::ProcessAddress current_vaddr) { LOG_ERROR(HW_Memory, @@ -256,14 +259,14 @@ struct Memory::Impl { }); } - void ReadBlock(const Common::ProcessAddress src_addr, void* dest_buffer, + bool ReadBlock(const Common::ProcessAddress src_addr, void* dest_buffer, const std::size_t size) { - ReadBlockImpl(*system.ApplicationProcess(), src_addr, dest_buffer, size); + return ReadBlockImpl(src_addr, dest_buffer, size); } - void ReadBlockUnsafe(const Common::ProcessAddress src_addr, void* dest_buffer, + bool ReadBlockUnsafe(const Common::ProcessAddress src_addr, void* dest_buffer, const std::size_t size) { - ReadBlockImpl(*system.ApplicationProcess(), src_addr, dest_buffer, size); + return ReadBlockImpl(src_addr, dest_buffer, size); } const u8* GetSpan(const VAddr src_addr, const std::size_t size) const { @@ -283,10 +286,10 @@ struct Memory::Impl { } template - void WriteBlockImpl(const Kernel::KProcess& process, const Common::ProcessAddress dest_addr, - const void* src_buffer, const std::size_t size) { - WalkBlock( - process, dest_addr, size, + bool WriteBlockImpl(const Common::ProcessAddress dest_addr, const void* src_buffer, + const std::size_t size) { + return WalkBlock( + dest_addr, size, [dest_addr, size](const std::size_t copy_amount, const Common::ProcessAddress current_vaddr) { LOG_ERROR(HW_Memory, @@ -308,20 +311,19 @@ struct Memory::Impl { }); } - void WriteBlock(const Common::ProcessAddress dest_addr, const void* src_buffer, + bool WriteBlock(const Common::ProcessAddress dest_addr, const void* src_buffer, const std::size_t size) { - WriteBlockImpl(*system.ApplicationProcess(), dest_addr, src_buffer, size); + return WriteBlockImpl(dest_addr, src_buffer, size); } - void WriteBlockUnsafe(const Common::ProcessAddress dest_addr, const void* src_buffer, + bool WriteBlockUnsafe(const Common::ProcessAddress dest_addr, const void* src_buffer, const std::size_t size) { - WriteBlockImpl(*system.ApplicationProcess(), dest_addr, src_buffer, size); + return WriteBlockImpl(dest_addr, src_buffer, size); } - void ZeroBlock(const Kernel::KProcess& process, const Common::ProcessAddress dest_addr, - const std::size_t size) { - WalkBlock( - process, dest_addr, size, + bool ZeroBlock(const Common::ProcessAddress dest_addr, const std::size_t size) { + return WalkBlock( + dest_addr, size, [dest_addr, size](const std::size_t copy_amount, const Common::ProcessAddress current_vaddr) { LOG_ERROR(HW_Memory, @@ -339,23 +341,23 @@ struct Memory::Impl { [](const std::size_t copy_amount) {}); } - void CopyBlock(const Kernel::KProcess& process, Common::ProcessAddress dest_addr, - Common::ProcessAddress src_addr, const std::size_t size) { - WalkBlock( - process, dest_addr, size, + bool CopyBlock(Common::ProcessAddress dest_addr, Common::ProcessAddress src_addr, + const std::size_t size) { + return WalkBlock( + dest_addr, size, [&](const std::size_t copy_amount, const Common::ProcessAddress current_vaddr) { LOG_ERROR(HW_Memory, "Unmapped CopyBlock @ 0x{:016X} (start address = 0x{:016X}, size = {})", GetInteger(current_vaddr), GetInteger(src_addr), size); - ZeroBlock(process, dest_addr, copy_amount); + ZeroBlock(dest_addr, copy_amount); }, [&](const std::size_t copy_amount, const u8* const src_ptr) { - WriteBlockImpl(process, dest_addr, src_ptr, copy_amount); + WriteBlockImpl(dest_addr, src_ptr, copy_amount); }, [&](const Common::ProcessAddress current_vaddr, const std::size_t copy_amount, u8* const host_ptr) { HandleRasterizerDownload(GetInteger(current_vaddr), copy_amount); - WriteBlockImpl(process, dest_addr, host_ptr, copy_amount); + WriteBlockImpl(dest_addr, host_ptr, copy_amount); }, [&](const std::size_t copy_amount) { dest_addr += copy_amount; @@ -364,13 +366,13 @@ struct Memory::Impl { } template - Result PerformCacheOperation(const Kernel::KProcess& process, Common::ProcessAddress dest_addr, - std::size_t size, Callback&& cb) { + Result PerformCacheOperation(Common::ProcessAddress dest_addr, std::size_t size, + Callback&& cb) { class InvalidMemoryException : public std::exception {}; try { WalkBlock( - process, dest_addr, size, + dest_addr, size, [&](const std::size_t block_size, const Common::ProcessAddress current_vaddr) { LOG_ERROR(HW_Memory, "Unmapped cache maintenance @ {:#018X}", GetInteger(current_vaddr)); @@ -387,37 +389,34 @@ struct Memory::Impl { return ResultSuccess; } - Result InvalidateDataCache(const Kernel::KProcess& process, Common::ProcessAddress dest_addr, - std::size_t size) { + Result InvalidateDataCache(Common::ProcessAddress dest_addr, std::size_t size) { auto on_rasterizer = [&](const Common::ProcessAddress current_vaddr, const std::size_t block_size) { // dc ivac: Invalidate to point of coherency // GPU flush -> CPU invalidate HandleRasterizerDownload(GetInteger(current_vaddr), block_size); }; - return PerformCacheOperation(process, dest_addr, size, on_rasterizer); + return PerformCacheOperation(dest_addr, size, on_rasterizer); } - Result StoreDataCache(const Kernel::KProcess& process, Common::ProcessAddress dest_addr, - std::size_t size) { + Result StoreDataCache(Common::ProcessAddress dest_addr, std::size_t size) { auto on_rasterizer = [&](const Common::ProcessAddress current_vaddr, const std::size_t block_size) { // dc cvac: Store to point of coherency // CPU flush -> GPU invalidate system.GPU().InvalidateRegion(GetInteger(current_vaddr), block_size); }; - return PerformCacheOperation(process, dest_addr, size, on_rasterizer); + return PerformCacheOperation(dest_addr, size, on_rasterizer); } - Result FlushDataCache(const Kernel::KProcess& process, Common::ProcessAddress dest_addr, - std::size_t size) { + Result FlushDataCache(Common::ProcessAddress dest_addr, std::size_t size) { auto on_rasterizer = [&](const Common::ProcessAddress current_vaddr, const std::size_t block_size) { // dc civac: Store to point of coherency, and invalidate from cache // CPU flush -> GPU invalidate system.GPU().InvalidateRegion(GetInteger(current_vaddr), block_size); }; - return PerformCacheOperation(process, dest_addr, size, on_rasterizer); + return PerformCacheOperation(dest_addr, size, on_rasterizer); } void MarkRegionDebug(u64 vaddr, u64 size, bool debug) { @@ -899,14 +898,14 @@ std::string Memory::ReadCString(Common::ProcessAddress vaddr, std::size_t max_le return impl->ReadCString(vaddr, max_length); } -void Memory::ReadBlock(const Common::ProcessAddress src_addr, void* dest_buffer, +bool Memory::ReadBlock(const Common::ProcessAddress src_addr, void* dest_buffer, const std::size_t size) { - impl->ReadBlock(src_addr, dest_buffer, size); + return impl->ReadBlock(src_addr, dest_buffer, size); } -void Memory::ReadBlockUnsafe(const Common::ProcessAddress src_addr, void* dest_buffer, +bool Memory::ReadBlockUnsafe(const Common::ProcessAddress src_addr, void* dest_buffer, const std::size_t size) { - impl->ReadBlockUnsafe(src_addr, dest_buffer, size); + return impl->ReadBlockUnsafe(src_addr, dest_buffer, size); } const u8* Memory::GetSpan(const VAddr src_addr, const std::size_t size) const { @@ -917,23 +916,23 @@ u8* Memory::GetSpan(const VAddr src_addr, const std::size_t size) { return impl->GetSpan(src_addr, size); } -void Memory::WriteBlock(const Common::ProcessAddress dest_addr, const void* src_buffer, +bool Memory::WriteBlock(const Common::ProcessAddress dest_addr, const void* src_buffer, const std::size_t size) { - impl->WriteBlock(dest_addr, src_buffer, size); + return impl->WriteBlock(dest_addr, src_buffer, size); } -void Memory::WriteBlockUnsafe(const Common::ProcessAddress dest_addr, const void* src_buffer, +bool Memory::WriteBlockUnsafe(const Common::ProcessAddress dest_addr, const void* src_buffer, const std::size_t size) { - impl->WriteBlockUnsafe(dest_addr, src_buffer, size); + return impl->WriteBlockUnsafe(dest_addr, src_buffer, size); } -void Memory::CopyBlock(Common::ProcessAddress dest_addr, Common::ProcessAddress src_addr, +bool Memory::CopyBlock(Common::ProcessAddress dest_addr, Common::ProcessAddress src_addr, const std::size_t size) { - impl->CopyBlock(*system.ApplicationProcess(), dest_addr, src_addr, size); + return impl->CopyBlock(dest_addr, src_addr, size); } -void Memory::ZeroBlock(Common::ProcessAddress dest_addr, const std::size_t size) { - impl->ZeroBlock(*system.ApplicationProcess(), dest_addr, size); +bool Memory::ZeroBlock(Common::ProcessAddress dest_addr, const std::size_t size) { + return impl->ZeroBlock(dest_addr, size); } void Memory::SetGPUDirtyManagers(std::span managers) { @@ -941,15 +940,15 @@ void Memory::SetGPUDirtyManagers(std::span managers } Result Memory::InvalidateDataCache(Common::ProcessAddress dest_addr, const std::size_t size) { - return impl->InvalidateDataCache(*system.ApplicationProcess(), dest_addr, size); + return impl->InvalidateDataCache(dest_addr, size); } Result Memory::StoreDataCache(Common::ProcessAddress dest_addr, const std::size_t size) { - return impl->StoreDataCache(*system.ApplicationProcess(), dest_addr, size); + return impl->StoreDataCache(dest_addr, size); } Result Memory::FlushDataCache(Common::ProcessAddress dest_addr, const std::size_t size) { - return impl->FlushDataCache(*system.ApplicationProcess(), dest_addr, size); + return impl->FlushDataCache(dest_addr, size); } void Memory::RasterizerMarkRegionCached(Common::ProcessAddress vaddr, u64 size, bool cached) { diff --git a/src/core/memory.h b/src/core/memory.h index ea33c769cf..2eb61ffd34 100644 --- a/src/core/memory.h +++ b/src/core/memory.h @@ -24,7 +24,6 @@ class GPUDirtyMemoryManager; } // namespace Core namespace Kernel { -class PhysicalMemory; class KProcess; } // namespace Kernel @@ -330,7 +329,7 @@ public: * @post The range [dest_buffer, size) contains the read bytes from the * current process' address space. */ - void ReadBlock(Common::ProcessAddress src_addr, void* dest_buffer, std::size_t size); + bool ReadBlock(Common::ProcessAddress src_addr, void* dest_buffer, std::size_t size); /** * Reads a contiguous block of bytes from the current process' address space. @@ -349,7 +348,7 @@ public: * @post The range [dest_buffer, size) contains the read bytes from the * current process' address space. */ - void ReadBlockUnsafe(Common::ProcessAddress src_addr, void* dest_buffer, std::size_t size); + bool ReadBlockUnsafe(Common::ProcessAddress src_addr, void* dest_buffer, std::size_t size); const u8* GetSpan(const VAddr src_addr, const std::size_t size) const; u8* GetSpan(const VAddr src_addr, const std::size_t size); @@ -373,7 +372,7 @@ public: * and will mark that region as invalidated to caches that the active * graphics backend may be maintaining over the course of execution. */ - void WriteBlock(Common::ProcessAddress dest_addr, const void* src_buffer, std::size_t size); + bool WriteBlock(Common::ProcessAddress dest_addr, const void* src_buffer, std::size_t size); /** * Writes a range of bytes into the current process' address space at the specified @@ -391,7 +390,7 @@ public: * will be ignored and an error will be logged. * */ - void WriteBlockUnsafe(Common::ProcessAddress dest_addr, const void* src_buffer, + bool WriteBlockUnsafe(Common::ProcessAddress dest_addr, const void* src_buffer, std::size_t size); /** @@ -405,7 +404,7 @@ public: * @post The range [dest_addr, size) within the process' address space contains the * same data within the range [src_addr, size). */ - void CopyBlock(Common::ProcessAddress dest_addr, Common::ProcessAddress src_addr, + bool CopyBlock(Common::ProcessAddress dest_addr, Common::ProcessAddress src_addr, std::size_t size); /** @@ -418,7 +417,7 @@ public: * @post The range [dest_addr, size) within the process' address space contains the * value 0. */ - void ZeroBlock(Common::ProcessAddress dest_addr, std::size_t size); + bool ZeroBlock(Common::ProcessAddress dest_addr, std::size_t size); /** * Invalidates a range of bytes within the current process' address space at the specified From 9f3f615e054663fd6e538fa2db86271b467a6bfd Mon Sep 17 00:00:00 2001 From: Liam Date: Fri, 14 Jul 2023 22:32:24 -0400 Subject: [PATCH 2/2] core: reduce TOCTTOU memory access --- src/core/debugger/gdbstub.cpp | 9 +++------ src/core/hle/kernel/svc/svc_ipc.cpp | 11 ++++------- src/core/hle/kernel/svc/svc_synchronization.cpp | 11 ++++------- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/src/core/debugger/gdbstub.cpp b/src/core/debugger/gdbstub.cpp index da6078372e..0f839d5b4c 100644 --- a/src/core/debugger/gdbstub.cpp +++ b/src/core/debugger/gdbstub.cpp @@ -261,10 +261,8 @@ void GDBStub::ExecuteCommand(std::string_view packet, std::vector(strtoll(command.data(), nullptr, 16))}; const size_t size{static_cast(strtoll(command.data() + sep, nullptr, 16))}; - if (system.ApplicationMemory().IsValidVirtualAddressRange(addr, size)) { - std::vector mem(size); - system.ApplicationMemory().ReadBlock(addr, mem.data(), size); - + std::vector mem(size); + if (system.ApplicationMemory().ReadBlock(addr, mem.data(), size)) { SendReply(Common::HexToString(mem)); } else { SendReply(GDB_STUB_REPLY_ERR); @@ -281,8 +279,7 @@ void GDBStub::ExecuteCommand(std::string_view packet, std::vector 0) { - // Ensure we can try to get the handles. - R_UNLESS(GetCurrentMemory(kernel).IsValidVirtualAddressRange( - handles_addr, static_cast(sizeof(Handle) * num_handles)), - ResultInvalidPointer); - // Get the handles. - GetCurrentMemory(kernel).ReadBlock(handles_addr, handles.data(), - sizeof(Handle) * num_handles); + R_UNLESS(GetCurrentMemory(kernel).ReadBlock(handles_addr, handles.data(), + sizeof(Handle) * num_handles), + ResultInvalidPointer); // Convert the handles to objects. R_UNLESS(handle_table.GetMultipleObjects( diff --git a/src/core/hle/kernel/svc/svc_synchronization.cpp b/src/core/hle/kernel/svc/svc_synchronization.cpp index f02d03f309..366e8ed4a2 100644 --- a/src/core/hle/kernel/svc/svc_synchronization.cpp +++ b/src/core/hle/kernel/svc/svc_synchronization.cpp @@ -7,6 +7,7 @@ #include "core/hle/kernel/k_process.h" #include "core/hle/kernel/k_readable_event.h" #include "core/hle/kernel/svc.h" +#include "core/hle/kernel/svc_results.h" namespace Kernel::Svc { @@ -64,14 +65,10 @@ Result WaitSynchronization(Core::System& system, int32_t* out_index, u64 user_ha // Copy user handles. if (num_handles > 0) { - // Ensure we can try to get the handles. - R_UNLESS(GetCurrentMemory(kernel).IsValidVirtualAddressRange( - user_handles, static_cast(sizeof(Handle) * num_handles)), - ResultInvalidPointer); - // Get the handles. - GetCurrentMemory(kernel).ReadBlock(user_handles, handles.data(), - sizeof(Handle) * num_handles); + R_UNLESS(GetCurrentMemory(kernel).ReadBlock(user_handles, handles.data(), + sizeof(Handle) * num_handles), + ResultInvalidPointer); // Convert the handles to objects. R_UNLESS(handle_table.GetMultipleObjects(