diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp index 93e6777bbe..04473d009c 100644 --- a/src/dawn_native/BindGroupLayout.cpp +++ b/src/dawn_native/BindGroupLayout.cpp @@ -148,6 +148,15 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("Binding type cannot be used with this visibility."); } + // Dynamic storage buffers aren't bounds checked properly in D3D12. Disallow them as + // unsafe until the bounds checks are implemented. + if (device->IsToggleEnabled(Toggle::DisallowUnsafeAPIs) && + entry.type == wgpu::BindingType::StorageBuffer && entry.hasDynamicOffset) { + return DAWN_VALIDATION_ERROR( + "Dynamic storage buffers are disallowed because they aren't secure yet. See " + "https://crbug.com/dawn/429"); + } + IncrementBindingCounts(&bindingCounts, entry); bindingsSet.insert(bindingNumber); diff --git a/src/dawn_native/ComputePassEncoder.cpp b/src/dawn_native/ComputePassEncoder.cpp index bfb0e7eac9..e421ea4aad 100644 --- a/src/dawn_native/ComputePassEncoder.cpp +++ b/src/dawn_native/ComputePassEncoder.cpp @@ -70,6 +70,14 @@ namespace dawn_native { mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { DAWN_TRY(GetDevice()->ValidateObject(indirectBuffer)); + // Indexed dispatches need a compute-shader based validation to check that the dispatch + // sizes aren't too big. Disallow them as unsafe until the validation is implemented. + if (GetDevice()->IsToggleEnabled(Toggle::DisallowUnsafeAPIs)) { + return DAWN_VALIDATION_ERROR( + "DispatchIndirect is disallowed because it doesn't validate that the dispatch " + "size is valid yet."); + } + if (indirectOffset % 4 != 0) { return DAWN_VALIDATION_ERROR("Indirect offset must be a multiple of 4"); } diff --git a/src/dawn_native/RenderEncoderBase.cpp b/src/dawn_native/RenderEncoderBase.cpp index c8182f6922..0ca5b436a0 100644 --- a/src/dawn_native/RenderEncoderBase.cpp +++ b/src/dawn_native/RenderEncoderBase.cpp @@ -112,6 +112,15 @@ namespace dawn_native { mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { DAWN_TRY(GetDevice()->ValidateObject(indirectBuffer)); + // Indexed indirect draws need a compute-shader based validation check that the range of + // indices is contained inside the index buffer on Metal. Disallow them as unsafe until + // the validation is implemented. + if (GetDevice()->IsToggleEnabled(Toggle::DisallowUnsafeAPIs)) { + return DAWN_VALIDATION_ERROR( + "DrawIndexedIndirect is disallowed because it doesn't validate that the index " + "range is valid yet."); + } + if (indirectOffset % 4 != 0) { return DAWN_VALIDATION_ERROR("Indirect offset must be a multiple of 4"); } diff --git a/src/dawn_native/Toggles.cpp b/src/dawn_native/Toggles.cpp index e98fd80834..e7bae9ec44 100644 --- a/src/dawn_native/Toggles.cpp +++ b/src/dawn_native/Toggles.cpp @@ -29,111 +29,118 @@ namespace dawn_native { using ToggleEnumAndInfoList = std::array(Toggle::EnumCount)>; - static constexpr ToggleEnumAndInfoList kToggleNameAndInfoList = { - {{Toggle::EmulateStoreAndMSAAResolve, - {"emulate_store_and_msaa_resolve", - "Emulate storing into multisampled color attachments and doing MSAA resolve " - "simultaneously. This workaround is enabled by default on the Metal drivers that do " - "not support MTLStoreActionStoreAndMultisampleResolve. To support StoreOp::Store on " - "those platforms, we should do MSAA resolve in another render pass after ending the " - "previous one.", - "https://crbug.com/dawn/56"}}, - {Toggle::NonzeroClearResourcesOnCreationForTesting, - {"nonzero_clear_resources_on_creation_for_testing", - "Clears texture to full 1 bits as soon as they are created, but doesn't update " - "the tracking state of the texture. This way we can test the logic of clearing " - "textures that use recycled memory.", - "https://crbug.com/dawn/145"}}, - {Toggle::AlwaysResolveIntoZeroLevelAndLayer, - {"always_resolve_into_zero_level_and_layer", - "When the resolve target is a texture view that is created on the non-zero level or " - "layer of a texture, we first resolve into a temporarily 2D texture with only one " - "mipmap level and one array layer, and copy the result of MSAA resolve into the " - "true resolve target. This workaround is enabled by default on the Metal drivers " - "that have bugs when setting non-zero resolveLevel or resolveSlice.", - "https://crbug.com/dawn/56"}}, - {Toggle::LazyClearResourceOnFirstUse, - {"lazy_clear_resource_on_first_use", - "Clears resource to zero on first usage. This initializes the resource " - "so that no dirty bits from recycled memory is present in the new resource.", - "https://crbug.com/dawn/145"}}, - {Toggle::TurnOffVsync, - {"turn_off_vsync", - "Turn off vsync when rendering. In order to do performance test or run perf tests, " - "turn off vsync so that the fps can exeed 60.", - "https://crbug.com/dawn/237"}}, - {Toggle::UseTemporaryBufferInCompressedTextureToTextureCopy, - {"use_temporary_buffer_in_texture_to_texture_copy", - "Split texture-to-texture copy into two copies: copy from source texture into a " - "temporary buffer, and copy from the temporary buffer into the destination texture " - "when copying between compressed textures that don't have block-aligned sizes. This " - "workaround is enabled by default on all Vulkan drivers to solve an issue in the " - "Vulkan SPEC about the texture-to-texture copies with compressed formats. See #1005 " - "(https://github.com/KhronosGroup/Vulkan-Docs/issues/1005) for more details.", - "https://crbug.com/dawn/42"}}, - {Toggle::UseD3D12ResourceHeapTier2, - {"use_d3d12_resource_heap_tier2", - "Enable support for resource heap tier 2. Resource heap tier 2 allows mixing of " - "texture and buffers in the same heap. This allows better heap re-use and reduces " - "fragmentation.", - "https://crbug.com/dawn/27"}}, - {Toggle::UseD3D12RenderPass, - {"use_d3d12_render_pass", - "Use the D3D12 render pass API introduced in Windows build 1809 by default. On " - "versions of Windows prior to build 1809, or when this toggle is turned off, Dawn " - "will emulate a render pass.", - "https://crbug.com/dawn/36"}}, - {Toggle::UseD3D12ResidencyManagement, - {"use_d3d12_residency_management", - "Enable residency management. This allows page-in and page-out of resource heaps in " - "GPU memory. This component improves overcommitted performance by keeping the most " - "recently used resources local to the GPU. Turning this component off can cause " - "allocation failures when application memory exceeds physical device memory.", - "https://crbug.com/dawn/193"}}, - {Toggle::SkipValidation, - {"skip_validation", "Skip expensive validation of Dawn commands.", - "https://crbug.com/dawn/271"}}, - {Toggle::VulkanUseD32S8, - {"vulkan_use_d32s8", - "Vulkan mandates support of either D32_FLOAT_S8 or D24_UNORM_S8. When available the " - "backend will use D32S8 (toggle to on) but setting the toggle to off will make it" - "use the D24S8 format when possible.", - "https://crbug.com/dawn/286"}}, - {Toggle::MetalDisableSamplerCompare, - {"metal_disable_sampler_compare", - "Disables the use of sampler compare on Metal. This is unsupported before A9 " - "processors.", - "https://crbug.com/dawn/342"}}, - {Toggle::MetalUseSharedModeForCounterSampleBuffer, - {"metal_use_shared_mode_for_counter_sample_buffer", - "The query set on Metal need to create MTLCounterSampleBuffer which storage mode " - "must be either MTLStorageModeShared or MTLStorageModePrivate. But the private mode " - "does not work properly on Intel platforms. The workaround is use shared mode " - "instead.", - "https://crbug.com/dawn/434"}}, - {Toggle::DisableBaseVertex, - {"disable_base_vertex", - "Disables the use of non-zero base vertex which is unsupported on some platforms.", - "https://crbug.com/dawn/343"}}, - {Toggle::DisableBaseInstance, - {"disable_base_instance", - "Disables the use of non-zero base instance which is unsupported on some " - "platforms.", - "https://crbug.com/dawn/343"}}, - {Toggle::UseD3D12SmallShaderVisibleHeapForTesting, - {"use_d3d12_small_shader_visible_heap", - "Enable use of a small D3D12 shader visible heap, instead of using a large one by " - "default. This setting is used to test bindgroup encoding.", - "https://crbug.com/dawn/155"}}, - {Toggle::UseDXC, - {"use_dxc", "Use DXC instead of FXC for compiling HLSL", - "https://crbug.com/dawn/402"}}, - {Toggle::DisableRobustness, - {"disable_robustness", "Disable robust buffer access", "https://crbug.com/dawn/480"}}, - {Toggle::MetalEnableVertexPulling, - {"metal_enable_vertex_pulling", - "Uses vertex pulling to protect out-of-bounds reads on Metal", - "https://crbug.com/dawn/480"}}}}; + static constexpr ToggleEnumAndInfoList kToggleNameAndInfoList = {{ + {Toggle::EmulateStoreAndMSAAResolve, + {"emulate_store_and_msaa_resolve", + "Emulate storing into multisampled color attachments and doing MSAA resolve " + "simultaneously. This workaround is enabled by default on the Metal drivers that do " + "not support MTLStoreActionStoreAndMultisampleResolve. To support StoreOp::Store on " + "those platforms, we should do MSAA resolve in another render pass after ending the " + "previous one.", + "https://crbug.com/dawn/56"}}, + {Toggle::NonzeroClearResourcesOnCreationForTesting, + {"nonzero_clear_resources_on_creation_for_testing", + "Clears texture to full 1 bits as soon as they are created, but doesn't update " + "the tracking state of the texture. This way we can test the logic of clearing " + "textures that use recycled memory.", + "https://crbug.com/dawn/145"}}, + {Toggle::AlwaysResolveIntoZeroLevelAndLayer, + {"always_resolve_into_zero_level_and_layer", + "When the resolve target is a texture view that is created on the non-zero level or " + "layer of a texture, we first resolve into a temporarily 2D texture with only one " + "mipmap level and one array layer, and copy the result of MSAA resolve into the " + "true resolve target. This workaround is enabled by default on the Metal drivers " + "that have bugs when setting non-zero resolveLevel or resolveSlice.", + "https://crbug.com/dawn/56"}}, + {Toggle::LazyClearResourceOnFirstUse, + {"lazy_clear_resource_on_first_use", + "Clears resource to zero on first usage. This initializes the resource " + "so that no dirty bits from recycled memory is present in the new resource.", + "https://crbug.com/dawn/145"}}, + {Toggle::TurnOffVsync, + {"turn_off_vsync", + "Turn off vsync when rendering. In order to do performance test or run perf tests, " + "turn off vsync so that the fps can exeed 60.", + "https://crbug.com/dawn/237"}}, + {Toggle::UseTemporaryBufferInCompressedTextureToTextureCopy, + {"use_temporary_buffer_in_texture_to_texture_copy", + "Split texture-to-texture copy into two copies: copy from source texture into a " + "temporary buffer, and copy from the temporary buffer into the destination texture " + "when copying between compressed textures that don't have block-aligned sizes. This " + "workaround is enabled by default on all Vulkan drivers to solve an issue in the " + "Vulkan SPEC about the texture-to-texture copies with compressed formats. See #1005 " + "(https://github.com/KhronosGroup/Vulkan-Docs/issues/1005) for more details.", + "https://crbug.com/dawn/42"}}, + {Toggle::UseD3D12ResourceHeapTier2, + {"use_d3d12_resource_heap_tier2", + "Enable support for resource heap tier 2. Resource heap tier 2 allows mixing of " + "texture and buffers in the same heap. This allows better heap re-use and reduces " + "fragmentation.", + "https://crbug.com/dawn/27"}}, + {Toggle::UseD3D12RenderPass, + {"use_d3d12_render_pass", + "Use the D3D12 render pass API introduced in Windows build 1809 by default. On " + "versions of Windows prior to build 1809, or when this toggle is turned off, Dawn " + "will emulate a render pass.", + "https://crbug.com/dawn/36"}}, + {Toggle::UseD3D12ResidencyManagement, + {"use_d3d12_residency_management", + "Enable residency management. This allows page-in and page-out of resource heaps in " + "GPU memory. This component improves overcommitted performance by keeping the most " + "recently used resources local to the GPU. Turning this component off can cause " + "allocation failures when application memory exceeds physical device memory.", + "https://crbug.com/dawn/193"}}, + {Toggle::SkipValidation, + {"skip_validation", "Skip expensive validation of Dawn commands.", + "https://crbug.com/dawn/271"}}, + {Toggle::VulkanUseD32S8, + {"vulkan_use_d32s8", + "Vulkan mandates support of either D32_FLOAT_S8 or D24_UNORM_S8. When available the " + "backend will use D32S8 (toggle to on) but setting the toggle to off will make it" + "use the D24S8 format when possible.", + "https://crbug.com/dawn/286"}}, + {Toggle::MetalDisableSamplerCompare, + {"metal_disable_sampler_compare", + "Disables the use of sampler compare on Metal. This is unsupported before A9 " + "processors.", + "https://crbug.com/dawn/342"}}, + {Toggle::MetalUseSharedModeForCounterSampleBuffer, + {"metal_use_shared_mode_for_counter_sample_buffer", + "The query set on Metal need to create MTLCounterSampleBuffer which storage mode " + "must be either MTLStorageModeShared or MTLStorageModePrivate. But the private mode " + "does not work properly on Intel platforms. The workaround is use shared mode " + "instead.", + "https://crbug.com/dawn/434"}}, + {Toggle::DisableBaseVertex, + {"disable_base_vertex", + "Disables the use of non-zero base vertex which is unsupported on some platforms.", + "https://crbug.com/dawn/343"}}, + {Toggle::DisableBaseInstance, + {"disable_base_instance", + "Disables the use of non-zero base instance which is unsupported on some " + "platforms.", + "https://crbug.com/dawn/343"}}, + {Toggle::UseD3D12SmallShaderVisibleHeapForTesting, + {"use_d3d12_small_shader_visible_heap", + "Enable use of a small D3D12 shader visible heap, instead of using a large one by " + "default. This setting is used to test bindgroup encoding.", + "https://crbug.com/dawn/155"}}, + {Toggle::UseDXC, + {"use_dxc", "Use DXC instead of FXC for compiling HLSL", + "https://crbug.com/dawn/402"}}, + {Toggle::DisableRobustness, + {"disable_robustness", "Disable robust buffer access", "https://crbug.com/dawn/480"}}, + {Toggle::MetalEnableVertexPulling, + {"metal_enable_vertex_pulling", + "Uses vertex pulling to protect out-of-bounds reads on Metal", + "https://crbug.com/dawn/480"}}, + {Toggle::DisallowUnsafeAPIs, + {"disallow_unsafe_apis", + "Produces validation errors on API entry points or parameter combinations that " + "aren't considered secure yet.", + "http://crbug.com/1138528"}} + // Dummy comment to separate the }} so it is clearer what to copy-paste to add a toggle. + }}; } // anonymous namespace diff --git a/src/dawn_native/Toggles.h b/src/dawn_native/Toggles.h index a84edc28d9..c6380cf4d6 100644 --- a/src/dawn_native/Toggles.h +++ b/src/dawn_native/Toggles.h @@ -43,6 +43,7 @@ namespace dawn_native { UseDXC, DisableRobustness, MetalEnableVertexPulling, + DisallowUnsafeAPIs, EnumCount, InvalidEnum = EnumCount, diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn index 474b663d70..ad45f05cdf 100644 --- a/src/tests/BUILD.gn +++ b/src/tests/BUILD.gn @@ -208,6 +208,7 @@ test("dawn_unittests") { "unittests/validation/TextureValidationTests.cpp", "unittests/validation/TextureViewValidationTests.cpp", "unittests/validation/ToggleValidationTests.cpp", + "unittests/validation/UnsafeAPIValidationTests.cpp", "unittests/validation/ValidationTest.cpp", "unittests/validation/ValidationTest.h", "unittests/validation/VertexBufferValidationTests.cpp", diff --git a/src/tests/unittests/validation/UnsafeAPIValidationTests.cpp b/src/tests/unittests/validation/UnsafeAPIValidationTests.cpp new file mode 100644 index 0000000000..318d4fd8ce --- /dev/null +++ b/src/tests/unittests/validation/UnsafeAPIValidationTests.cpp @@ -0,0 +1,169 @@ +// Copyright 2020 The Dawn Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tests/unittests/validation/ValidationTest.h" + +#include "utils/ComboRenderBundleEncoderDescriptor.h" +#include "utils/ComboRenderPipelineDescriptor.h" +#include "utils/WGPUHelpers.h" + +class UnsafeAPIValidationTest : public ValidationTest { + protected: + wgpu::Device CreateTestDevice() override { + dawn_native::DeviceDescriptor descriptor; + descriptor.forceEnabledToggles.push_back("disallow_unsafe_apis"); + return wgpu::Device::Acquire(adapter.CreateDevice(&descriptor)); + } +}; + +// Check that DrawIndexedIndirect is disallowed as part of unsafe APIs. +TEST_F(UnsafeAPIValidationTest, DrawIndexedIndirectDisallowed) { + // Create the index and indirect buffers. + wgpu::BufferDescriptor indexBufferDesc; + indexBufferDesc.size = 4; + indexBufferDesc.usage = wgpu::BufferUsage::Index; + wgpu::Buffer indexBuffer = device.CreateBuffer(&indexBufferDesc); + + wgpu::BufferDescriptor indirectBufferDesc; + indirectBufferDesc.size = 64; + indirectBufferDesc.usage = wgpu::BufferUsage::Indirect; + wgpu::Buffer indirectBuffer = device.CreateBuffer(&indirectBufferDesc); + + // The RenderPassDescriptor, RenderBundleDescriptor and pipeline for all sub-tests below. + DummyRenderPass renderPass(device); + + utils::ComboRenderBundleEncoderDescriptor bundleDesc = {}; + bundleDesc.colorFormatsCount = 1; + bundleDesc.cColorFormats[0] = renderPass.attachmentFormat; + + utils::ComboRenderPipelineDescriptor desc(device); + desc.vertexStage.module = utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, + "#version 450\nvoid main() {}"); + desc.cFragmentStage.module = utils::CreateShaderModule( + device, utils::SingleShaderStage::Fragment, "#version 450\nvoid main() {}"); + wgpu::RenderPipeline pipeline = device.CreateRenderPipeline(&desc); + + // Control cases: DrawIndirect and DrawIndexed are allowed inside a render pass. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + pass.SetPipeline(pipeline); + + pass.SetIndexBufferWithFormat(indexBuffer, wgpu::IndexFormat::Uint32); + pass.DrawIndexed(1); + + pass.DrawIndirect(indirectBuffer, 0); + pass.EndPass(); + encoder.Finish(); + } + + // Control case: DrawIndirect and DrawIndexed are allowed inside a render bundle. + { + wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&bundleDesc); + encoder.SetPipeline(pipeline); + + encoder.SetIndexBufferWithFormat(indexBuffer, wgpu::IndexFormat::Uint32); + encoder.DrawIndexed(1); + + encoder.DrawIndirect(indirectBuffer, 0); + encoder.Finish(); + } + + // Error case, DrawIndexedIndirect is disallowed inside a render pass. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + + pass.SetPipeline(pipeline); + pass.SetIndexBufferWithFormat(indexBuffer, wgpu::IndexFormat::Uint32); + pass.DrawIndexedIndirect(indirectBuffer, 0); + + pass.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + // Error case, DrawIndexedIndirect is disallowed inside a render bundle. + { + wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&bundleDesc); + + encoder.SetPipeline(pipeline); + encoder.SetIndexBufferWithFormat(indexBuffer, wgpu::IndexFormat::Uint32); + encoder.DrawIndexedIndirect(indirectBuffer, 0); + + ASSERT_DEVICE_ERROR(encoder.Finish()); + } +} + +// Check that DispatchIndirect is disallowed as part of unsafe APIs. +TEST_F(UnsafeAPIValidationTest, DispatchIndirectDisallowed) { + // Create the index and indirect buffers. + wgpu::BufferDescriptor indirectBufferDesc; + indirectBufferDesc.size = 64; + indirectBufferDesc.usage = wgpu::BufferUsage::Indirect; + wgpu::Buffer indirectBuffer = device.CreateBuffer(&indirectBufferDesc); + + // Create the dummy compute pipeline. + wgpu::ComputePipelineDescriptor pipelineDesc; + pipelineDesc.computeStage.entryPoint = "main"; + pipelineDesc.computeStage.module = utils::CreateShaderModule( + device, utils::SingleShaderStage::Compute, "#version 450\nvoid main() {}"); + wgpu::ComputePipeline pipeline = device.CreateComputePipeline(&pipelineDesc); + + // Control case: dispatch is allowed. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::ComputePassEncoder pass = encoder.BeginComputePass(); + + pass.SetPipeline(pipeline); + pass.Dispatch(1, 1, 1); + + pass.EndPass(); + encoder.Finish(); + } + + // Error case: dispatch indirect is disallowed. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::ComputePassEncoder pass = encoder.BeginComputePass(); + + pass.SetPipeline(pipeline); + pass.DispatchIndirect(indirectBuffer, 0); + + pass.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } +} + +// Check that dynamic storage buffers are disallowed. +TEST_F(UnsafeAPIValidationTest, DynamicStorageBuffer) { + wgpu::BindGroupLayoutEntry entry; + entry.type = wgpu::BindingType::StorageBuffer; + entry.visibility = wgpu::ShaderStage::Fragment; + + wgpu::BindGroupLayoutDescriptor desc; + desc.entries = &entry; + desc.entryCount = 1; + + // Control case: storage buffer without a dynamic offset is allowed. + { + entry.hasDynamicOffset = false; + device.CreateBindGroupLayout(&desc); + } + + // Control case: storage buffer with a dynamic offset is disallowed. + { + entry.hasDynamicOffset = true; + ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&desc)); + } +}