From 11c0f579b1d2f568c436f81e41cb508fd93afbc1 Mon Sep 17 00:00:00 2001 From: Tomek Ponitka Date: Tue, 11 Aug 2020 12:04:52 +0000 Subject: [PATCH] Adding validation for requiredBytesInCopy overflow Also some uint32_t computations are now done on uint64_t. Bug: dawn:482 Change-Id: Ia0094e16999ec3a9fec193f27760e73cd14d289c Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/26540 Reviewed-by: Corentin Wallez Reviewed-by: Jiawei Shao Commit-Queue: Tomek Ponitka --- src/dawn_native/CommandValidation.cpp | 72 ++++++++++++------- src/dawn_native/CommandValidation.h | 8 +-- src/dawn_native/Queue.cpp | 2 +- src/dawn_native/d3d12/QueueD3D12.cpp | 7 +- src/dawn_native/metal/QueueMTL.mm | 6 +- src/dawn_native/vulkan/QueueVk.cpp | 7 +- .../CopyCommandsValidationTests.cpp | 28 ++++++++ .../QueueWriteTextureValidationTests.cpp | 12 ++++ 8 files changed, 104 insertions(+), 38 deletions(-) diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp index 034585a527..7b2d8fc597 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp @@ -370,15 +370,20 @@ namespace dawn_native { static_cast(maxStart); } - uint32_t ComputeRequiredBytesInCopy(const TexelBlockInfo& blockInfo, - const Extent3D& copySize, - uint32_t bytesPerRow, - uint32_t rowsPerImage) { + ResultOrError ComputeRequiredBytesInCopy(const TexelBlockInfo& blockInfo, + const Extent3D& copySize, + uint32_t bytesPerRow, + uint32_t rowsPerImage) { // Default value for rowsPerImage if (rowsPerImage == 0) { rowsPerImage = copySize.height; } + ASSERT(rowsPerImage >= copySize.height); + if (copySize.height > 1 || copySize.depth > 1) { + ASSERT(bytesPerRow >= copySize.width / blockInfo.blockWidth * blockInfo.blockByteSize); + } + if (copySize.width == 0 || copySize.height == 0 || copySize.depth == 0) { return 0; } @@ -386,11 +391,23 @@ namespace dawn_native { ASSERT(copySize.height >= 1); ASSERT(copySize.depth >= 1); - uint64_t texelBlockRowsPerImage = rowsPerImage / blockInfo.blockHeight; - uint64_t bytesPerImage = bytesPerRow * texelBlockRowsPerImage; + uint32_t texelBlockRowsPerImage = rowsPerImage / blockInfo.blockHeight; + // bytesPerImage won't overflow since we're multiplying two uint32_t numbers + uint64_t bytesPerImage = uint64_t(texelBlockRowsPerImage) * bytesPerRow; + // Provided that copySize.height > 1: bytesInLastSlice won't overflow since it's at most + // bytesPerImage. Otherwise the result is a multiplication of two uint32_t numbers. uint64_t bytesInLastSlice = - bytesPerRow * (copySize.height / blockInfo.blockHeight - 1) + - (copySize.width / blockInfo.blockWidth * blockInfo.blockByteSize); + uint64_t(bytesPerRow) * (copySize.height / blockInfo.blockHeight - 1) + + (uint64_t(copySize.width) / blockInfo.blockWidth * blockInfo.blockByteSize); + + // This error cannot be thrown for copySize.depth = 1. + // For copySize.depth > 1 we know that: + // requiredBytesInCopy >= (copySize.depth * bytesPerImage) / 2, so if + // copySize.depth * bytesPerImage overflows uint64_t, then requiredBytesInCopy is definitely + // too large to fit in the available data size. + if (std::numeric_limits::max() / copySize.depth < bytesPerImage) { + return DAWN_VALIDATION_ERROR("requiredBytesInCopy is too large"); + } return bytesPerImage * (copySize.depth - 1) + bytesInLastSlice; } @@ -420,25 +437,6 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("Offset must be a multiple of the texel or block size"); } - // Validation for the copy being in-bounds: - if (layout.rowsPerImage != 0 && layout.rowsPerImage < copyExtent.height) { - return DAWN_VALIDATION_ERROR("rowsPerImage must not be less than the copy height."); - } - - // We compute required bytes in copy after validating texel block alignments - // because the divisibility conditions are necessary for the algorithm to be valid. - // TODO(tommek@google.com): to match the spec this should only be checked when - // copyExtent.depth > 1. - uint32_t requiredBytesInCopy = ComputeRequiredBytesInCopy( - blockInfo, copyExtent, layout.bytesPerRow, layout.rowsPerImage); - - bool fitsInData = - layout.offset <= byteSize && (requiredBytesInCopy <= (byteSize - layout.offset)); - if (!fitsInData) { - return DAWN_VALIDATION_ERROR( - "Required size for texture data layout exceeds the given size"); - } - // Validation for other members in layout: if ((copyExtent.height > 1 || copyExtent.depth > 1) && layout.bytesPerRow < @@ -450,6 +448,26 @@ namespace dawn_native { // TODO(tommek@google.com): to match the spec there should be another condition here // on rowsPerImage >= copyExtent.height if copyExtent.depth > 1. + // Validation for the copy being in-bounds: + if (layout.rowsPerImage != 0 && layout.rowsPerImage < copyExtent.height) { + return DAWN_VALIDATION_ERROR("rowsPerImage must not be less than the copy height."); + } + + // We compute required bytes in copy after validating texel block alignments + // because the divisibility conditions are necessary for the algorithm to be valid, + // also the bytesPerRow bound is necessary to avoid overflows. + uint64_t requiredBytesInCopy; + DAWN_TRY_ASSIGN(requiredBytesInCopy, + ComputeRequiredBytesInCopy(blockInfo, copyExtent, layout.bytesPerRow, + layout.rowsPerImage)); + + bool fitsInData = + layout.offset <= byteSize && (requiredBytesInCopy <= (byteSize - layout.offset)); + if (!fitsInData) { + return DAWN_VALIDATION_ERROR( + "Required size for texture data layout exceeds the given size"); + } + return {}; } diff --git a/src/dawn_native/CommandValidation.h b/src/dawn_native/CommandValidation.h index 719ce17998..0aaa398ade 100644 --- a/src/dawn_native/CommandValidation.h +++ b/src/dawn_native/CommandValidation.h @@ -41,10 +41,10 @@ namespace dawn_native { MaybeError ValidateTimestampQuery(QuerySetBase* querySet, uint32_t queryIndex); - uint32_t ComputeRequiredBytesInCopy(const TexelBlockInfo& blockInfo, - const Extent3D& copySize, - uint32_t bytesPerRow, - uint32_t rowsPerImage); + ResultOrError ComputeRequiredBytesInCopy(const TexelBlockInfo& blockInfo, + const Extent3D& copySize, + uint32_t bytesPerRow, + uint32_t rowsPerImage); MaybeError ValidateLinearTextureData(const TextureDataLayout& layout, uint64_t byteSize, diff --git a/src/dawn_native/Queue.cpp b/src/dawn_native/Queue.cpp index 403e022133..435d2654e3 100644 --- a/src/dawn_native/Queue.cpp +++ b/src/dawn_native/Queue.cpp @@ -314,7 +314,7 @@ namespace dawn_native { srcPointer += imageAdditionalStride; } } else { - uint64_t layerSize = rowsPerImageInBlock * actualBytesPerRow; + uint64_t layerSize = uint64_t(rowsPerImageInBlock) * actualBytesPerRow; if (!copyWholeData) { // copy layer by layer for (uint32_t d = 0; d < depth; ++d) { memcpy(dstPointer, srcPointer, layerSize); diff --git a/src/dawn_native/d3d12/QueueD3D12.cpp b/src/dawn_native/d3d12/QueueD3D12.cpp index c880892d64..c7c097853d 100644 --- a/src/dawn_native/d3d12/QueueD3D12.cpp +++ b/src/dawn_native/d3d12/QueueD3D12.cpp @@ -37,8 +37,11 @@ namespace dawn_native { namespace d3d12 { const TextureDataLayout& dataLayout, const Format& textureFormat, const Extent3D& writeSizePixel) { - uint32_t newDataSizeBytes = ComputeRequiredBytesInCopy( - textureFormat, writeSizePixel, optimallyAlignedBytesPerRow, alignedRowsPerImage); + uint64_t newDataSizeBytes; + DAWN_TRY_ASSIGN( + newDataSizeBytes, + ComputeRequiredBytesInCopy(textureFormat, writeSizePixel, + optimallyAlignedBytesPerRow, alignedRowsPerImage)); UploadHandle uploadHandle; DAWN_TRY_ASSIGN(uploadHandle, device->GetDynamicUploader()->Allocate( diff --git a/src/dawn_native/metal/QueueMTL.mm b/src/dawn_native/metal/QueueMTL.mm index 44cbb0fbdf..07b1f79ea9 100644 --- a/src/dawn_native/metal/QueueMTL.mm +++ b/src/dawn_native/metal/QueueMTL.mm @@ -34,8 +34,10 @@ namespace dawn_native { namespace metal { const TextureDataLayout& dataLayout, const TexelBlockInfo& blockInfo, const Extent3D& writeSizePixel) { - uint32_t newDataSizeBytes = ComputeRequiredBytesInCopy( - blockInfo, writeSizePixel, alignedBytesPerRow, alignedRowsPerImage); + uint64_t newDataSizeBytes; + DAWN_TRY_ASSIGN(newDataSizeBytes, + ComputeRequiredBytesInCopy(blockInfo, writeSizePixel, + alignedBytesPerRow, alignedRowsPerImage)); UploadHandle uploadHandle; DAWN_TRY_ASSIGN(uploadHandle, device->GetDynamicUploader()->Allocate( diff --git a/src/dawn_native/vulkan/QueueVk.cpp b/src/dawn_native/vulkan/QueueVk.cpp index 3842652e72..8d70fd534c 100644 --- a/src/dawn_native/vulkan/QueueVk.cpp +++ b/src/dawn_native/vulkan/QueueVk.cpp @@ -37,8 +37,11 @@ namespace dawn_native { namespace vulkan { const TextureDataLayout& dataLayout, const TexelBlockInfo& blockInfo, const Extent3D& writeSizePixel) { - uint32_t newDataSizeBytes = ComputeRequiredBytesInCopy( - blockInfo, writeSizePixel, optimallyAlignedBytesPerRow, alignedRowsPerImage); + uint64_t newDataSizeBytes; + DAWN_TRY_ASSIGN( + newDataSizeBytes, + ComputeRequiredBytesInCopy(blockInfo, writeSizePixel, optimallyAlignedBytesPerRow, + alignedRowsPerImage)); uint64_t optimalOffsetAlignment = ToBackend(device) diff --git a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp index 3d4f65c871..52c242d846 100644 --- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp +++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp @@ -793,6 +793,20 @@ TEST_F(CopyCommandTest_B2T, CopyToStencilAspect) { } } +// Test that CopyB2T throws an error when requiredBytesInCopy overflows uint64_t +TEST_F(CopyCommandTest_B2T, RequiredBytesInCopyOverflow) { + wgpu::Buffer source = CreateBuffer(10000, wgpu::BufferUsage::CopySrc); + wgpu::Texture destination = + Create2DTexture(1, 1, 1, 16, wgpu::TextureFormat::RGBA8Unorm, wgpu::TextureUsage::CopyDst); + + // Success + TestB2TCopy(utils::Expectation::Success, source, 0, (1 << 31), (1 << 31), destination, 0, + {0, 0, 0}, {1, 1, 1}); + // Failure because bytesPerImage * (depth - 1) overflows + TestB2TCopy(utils::Expectation::Failure, source, 0, (1 << 31), (1 << 31), destination, 0, + {0, 0, 0}, {1, 1, 16}); +} + class CopyCommandTest_T2B : public CopyCommandTest {}; // Test a successfull T2B copy @@ -1227,6 +1241,20 @@ TEST_F(CopyCommandTest_T2B, CopyFromStencilAspect) { } } +// Test that CopyT2B throws an error when requiredBytesInCopy overflows uint64_t +TEST_F(CopyCommandTest_T2B, RequiredBytesInCopyOverflow) { + wgpu::Buffer destination = CreateBuffer(10000, wgpu::BufferUsage::CopyDst); + wgpu::Texture source = + Create2DTexture(1, 1, 1, 16, wgpu::TextureFormat::RGBA8Unorm, wgpu::TextureUsage::CopySrc); + + // Success + TestT2BCopy(utils::Expectation::Success, source, 0, {0, 0, 0}, destination, 0, (1 << 31), + (1 << 31), {1, 1, 1}); + // Failure because bytesPerImage * (depth - 1) overflows + TestT2BCopy(utils::Expectation::Failure, source, 0, {0, 0, 0}, destination, 0, (1 << 31), + (1 << 31), {1, 1, 16}); +} + class CopyCommandTest_T2T : public CopyCommandTest {}; TEST_F(CopyCommandTest_T2T, Success) { diff --git a/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp b/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp index d0aad6170e..ec65bf65f5 100644 --- a/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp +++ b/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp @@ -334,6 +334,18 @@ namespace { } } + // Test that WriteTexture throws an error when requiredBytesInCopy overflows uint64_t + TEST_F(QueueWriteTextureValidationTest, RequiredBytesInCopyOverflow) { + wgpu::Texture destination = Create2DTexture({1, 1, 16}, 1, wgpu::TextureFormat::RGBA8Unorm, + wgpu::TextureUsage::CopyDst); + + // success because depth = 1. + TestWriteTexture(10000, 0, (1 << 31), (1 << 31), destination, 0, {0, 0, 0}, {1, 1, 1}); + // failure because bytesPerImage * (depth - 1) overflows. + ASSERT_DEVICE_ERROR(TestWriteTexture(10000, 0, (1 << 31), (1 << 31), destination, 0, + {0, 0, 0}, {1, 1, 16})); + } + // Regression tests for a bug in the computation of texture data size in Dawn. TEST_F(QueueWriteTextureValidationTest, TextureWriteDataSizeLastRowComputation) { constexpr uint32_t kBytesPerRow = 256;