From b711a0576cab581553bff9e5e28413c09a1179f6 Mon Sep 17 00:00:00 2001 From: baldurk Date: Mon, 6 Nov 2023 15:07:10 +0000 Subject: [PATCH] Limit how much we allow D3D12 addresses to go out of bounds --- renderdoc/driver/d3d12/d3d12_debug.cpp | 2 +- renderdoc/driver/d3d12/d3d12_device.cpp | 4 +-- .../d3d12/d3d12_device_rescreate_wrap.cpp | 21 +++++++++++---- renderdoc/driver/d3d12/d3d12_manager.cpp | 7 ++++- renderdoc/driver/d3d12/d3d12_manager.h | 2 +- renderdoc/driver/d3d12/d3d12_resources.cpp | 2 +- renderdoc/driver/d3d12/d3d12_resources.h | 26 ++++++++++++++----- 7 files changed, 46 insertions(+), 18 deletions(-) diff --git a/renderdoc/driver/d3d12/d3d12_debug.cpp b/renderdoc/driver/d3d12/d3d12_debug.cpp index 9953592cbe..f8ed7123d2 100644 --- a/renderdoc/driver/d3d12/d3d12_debug.cpp +++ b/renderdoc/driver/d3d12/d3d12_debug.cpp @@ -1796,7 +1796,7 @@ void D3D12DebugManager::PrepareExecuteIndirectPatching(const GPUAddressRangeTrac { buffermapping b = {}; b.origBase = addr.start; - b.origEnd = addr.end; + b.origEnd = addr.realEnd; b.newBase = m_pDevice->GetResourceManager()->GetLiveAs(addr.id)->GetGPUVirtualAddress(); buffers.push_back(b); diff --git a/renderdoc/driver/d3d12/d3d12_device.cpp b/renderdoc/driver/d3d12/d3d12_device.cpp index 9a998347ae..fead16aa4d 100644 --- a/renderdoc/driver/d3d12/d3d12_device.cpp +++ b/renderdoc/driver/d3d12/d3d12_device.cpp @@ -1677,7 +1677,7 @@ bool WrappedID3D12Device::Serialise_WrapSwapchainBuffer(SerialiserType &ser, IDX } else { - WrappedID3D12Resource *wrapped = new WrappedID3D12Resource(fakeBB, this); + WrappedID3D12Resource *wrapped = new WrappedID3D12Resource(fakeBB, NULL, 0, this); fakeBB = wrapped; fakeBB->SetName(L"Swap Chain Buffer"); @@ -1719,7 +1719,7 @@ IUnknown *WrappedID3D12Device::WrapSwapchainBuffer(IDXGISwapper *swapper, DXGI_F } else { - pRes = new WrappedID3D12Resource((ID3D12Resource *)realSurface, this); + pRes = new WrappedID3D12Resource((ID3D12Resource *)realSurface, NULL, 0, this); ResourceId id = GetResID(pRes); diff --git a/renderdoc/driver/d3d12/d3d12_device_rescreate_wrap.cpp b/renderdoc/driver/d3d12/d3d12_device_rescreate_wrap.cpp index 7f6924ce16..3509b6c014 100644 --- a/renderdoc/driver/d3d12/d3d12_device_rescreate_wrap.cpp +++ b/renderdoc/driver/d3d12/d3d12_device_rescreate_wrap.cpp @@ -75,7 +75,20 @@ bool WrappedID3D12Device::Serialise_CreateResource( { GPUAddressRange range; range.start = gpuAddress; - range.end = gpuAddress + desc.Width; + range.realEnd = gpuAddress + desc.Width; + + // if this is placed, the OOB end is all the way to the end of the heap, from where we're + // placed, allowing accesses past the buffer but still in bounds of the heap. + if(pHeap) + { + const UINT64 heapSize = pHeap->GetDesc().SizeInBytes; + range.oobEnd = gpuAddress + (heapSize - HeapOffset); + } + else + { + range.oobEnd = range.realEnd; + } + range.id = pResource; m_OrigGPUAddresses.AddTo(range); @@ -232,7 +245,7 @@ bool WrappedID3D12Device::Serialise_CreateResource( SetObjName(ret, StringFormat::Fmt("%s Resource %s %s", ResourceTypeName, ToStr(desc.Dimension).c_str(), ToStr(pResource).c_str())); - ret = new WrappedID3D12Resource(ret, this); + ret = new WrappedID3D12Resource(ret, pHeap, HeapOffset, this); switch(chunkType) { @@ -422,9 +435,7 @@ HRESULT WrappedID3D12Device::CreateResource( UINT NumSubresources = GetNumSubresources(m_pDevice, &desc); - WrappedID3D12Resource *wrapped = new WrappedID3D12Resource(realRes, this); - - wrapped->SetHeap(pHeap); + WrappedID3D12Resource *wrapped = new WrappedID3D12Resource(realRes, pHeap, HeapOffset, this); if(IsCaptureMode(m_State)) { diff --git a/renderdoc/driver/d3d12/d3d12_manager.cpp b/renderdoc/driver/d3d12/d3d12_manager.cpp index e24839cc4f..241f971ff6 100644 --- a/renderdoc/driver/d3d12/d3d12_manager.cpp +++ b/renderdoc/driver/d3d12/d3d12_manager.cpp @@ -1041,7 +1041,7 @@ void GPUAddressRangeTracker::GetResIDFromAddr(D3D12_GPU_VIRTUAL_ADDRESS addr, Re range = *it; } - if(addr < range.start || addr >= range.end) + if(addr < range.start || addr >= range.realEnd) return; id = range.id; @@ -1073,6 +1073,11 @@ void GPUAddressRangeTracker::GetResIDFromAddrAllowOutOfBounds(D3D12_GPU_VIRTUAL_ if(addr < range.start) return; + // still enforce the OOB end on ranges - which is the remaining range in the backing store. + // Otherwise we could end up passing through invalid addresses stored in stale descriptors + if(addr >= range.oobEnd) + return; + id = range.id; offs = addr - range.start; } diff --git a/renderdoc/driver/d3d12/d3d12_manager.h b/renderdoc/driver/d3d12/d3d12_manager.h index 73d2a4af53..bdcf50b419 100644 --- a/renderdoc/driver/d3d12/d3d12_manager.h +++ b/renderdoc/driver/d3d12/d3d12_manager.h @@ -504,7 +504,7 @@ class WrappedID3D12Resource; struct GPUAddressRange { - D3D12_GPU_VIRTUAL_ADDRESS start, end; + D3D12_GPU_VIRTUAL_ADDRESS start, realEnd, oobEnd; ResourceId id; bool operator<(const D3D12_GPU_VIRTUAL_ADDRESS &o) const diff --git a/renderdoc/driver/d3d12/d3d12_resources.cpp b/renderdoc/driver/d3d12/d3d12_resources.cpp index ed4ca525e8..2e35f809ad 100644 --- a/renderdoc/driver/d3d12/d3d12_resources.cpp +++ b/renderdoc/driver/d3d12/d3d12_resources.cpp @@ -166,7 +166,7 @@ WrappedID3D12Resource::~WrappedID3D12Resource() { GPUAddressRange range; range.start = m_pReal->GetGPUVirtualAddress(); - range.end = m_pReal->GetGPUVirtualAddress() + m_pReal->GetDesc().Width; + // realEnd and oobEnd are not used for removing, just start + id range.id = GetResourceID(); m_Addresses.RemoveFrom(range); diff --git a/renderdoc/driver/d3d12/d3d12_resources.h b/renderdoc/driver/d3d12/d3d12_resources.h index a41b427b73..6692b9faa1 100644 --- a/renderdoc/driver/d3d12/d3d12_resources.h +++ b/renderdoc/driver/d3d12/d3d12_resources.h @@ -953,11 +953,6 @@ class WrappedID3D12Resource return this->GetResourceID(); } - void SetHeap(ID3D12Heap *heap) - { - m_Heap = (WrappedID3D12Heap *)heap; - SAFE_ADDREF(m_Heap); - } static void RefBuffers(D3D12ResourceManager *rm); static void GetMappableIDs(D3D12ResourceManager *rm, const std::unordered_set &refdIDs, std::unordered_set &mappableIDs); @@ -991,12 +986,16 @@ class WrappedID3D12Resource TypeEnum = Resource_Resource, }; - WrappedID3D12Resource(ID3D12Resource *real, WrappedID3D12Device *device) + WrappedID3D12Resource(ID3D12Resource *real, ID3D12Heap *heap, UINT64 HeapOffset, + WrappedID3D12Device *device) : WrappedDeviceChild12(real, device) { if(IsReplayMode(device->GetState())) device->AddReplayResource(GetResourceID(), this); + m_Heap = (WrappedID3D12Heap *)heap; + SAFE_ADDREF(m_Heap); + // assuming only valid for buffers if(m_pReal->GetDesc().Dimension == D3D12_RESOURCE_DIMENSION_BUFFER) { @@ -1004,7 +1003,20 @@ class WrappedID3D12Resource GPUAddressRange range; range.start = addr; - range.end = addr + m_pReal->GetDesc().Width; + range.realEnd = addr + m_pReal->GetDesc().Width; + + // if this is placed, the OOB end is all the way to the end of the heap, from where we're + // placed, allowing accesses past the buffer but still in bounds of the heap. + if(heap) + { + const UINT64 heapSize = heap->GetDesc().SizeInBytes; + range.oobEnd = addr + (heapSize - HeapOffset); + } + else + { + range.oobEnd = range.realEnd; + } + range.id = GetResourceID(); m_Addresses.AddTo(range);