Vulkan: fix potential OOB read in reformatStagedBufferUpdates Under certain drivers, it was possible for an OOB read to occur because reformatStagedBufferUpdates did not use layerCount for 2D array textures instead of depth when sizing the staged buffer. Skip on Intel Metal for now as the test fails there. Possibly a similar bug there that needs fixing. Bug: b/499091328 Change-Id: Ide82dc9eada013d0a11da199bd5857da9f223205 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/7818817 Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org> Reviewed-by: Charlie Lao <cclao@google.com> Commit-Queue: Antonio Maiorano <amaiorano@google.com>
diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.cpp b/src/libANGLE/renderer/vulkan/vk_helpers.cpp index 064cd64..1429619 100644 --- a/src/libANGLE/renderer/vulkan/vk_helpers.cpp +++ b/src/libANGLE/renderer/vulkan/vk_helpers.cpp
@@ -8826,6 +8826,10 @@ const size_t srcDataDepthPitch = srcDataRowPitch * copy.imageExtent.height; const size_t dstDataDepthPitch = dstDataRowPitch * copy.imageExtent.height; + const uint32_t depthOrLayerCount = mImageType == VK_IMAGE_TYPE_3D + ? copy.imageExtent.depth + : copy.imageSubresource.layerCount; + // Retrieve source buffer vk::BufferHelper *srcBuffer = update.data.buffer.bufferHelper; ASSERT(srcBuffer->isMapped()); @@ -8840,7 +8844,7 @@ uint8_t *dstData; VkDeviceSize dstBufferOffset; - size_t dstBufferSize = dstDataDepthPitch * copy.imageExtent.depth; + size_t dstBufferSize = dstDataDepthPitch * depthOrLayerCount; ANGLE_TRY(contextVk->initBufferForImageCopy( dstBuffer, dstBufferSize, MemoryCoherency::CachedNonCoherent, dstFormatID, &dstBufferOffset, &dstData)); @@ -8852,8 +8856,7 @@ pixelReadFunction, dstData, dstDataRowPitch, dstFormat.pixelBytes, dstDataDepthPitch, pixelWriteFunction, dstFormatInfo.format, dstFormatInfo.componentType, copy.imageExtent.width, - copy.imageExtent.height, copy.imageExtent.depth, false, false, - false); + copy.imageExtent.height, depthOrLayerCount, false, false, false); // Replace srcBuffer with dstBuffer update.data.buffer.bufferHelper = dstBuffer;
diff --git a/src/tests/angle_end2end_tests_expectations.txt b/src/tests/angle_end2end_tests_expectations.txt index 92486f2..a6d325e 100644 --- a/src/tests/angle_end2end_tests_expectations.txt +++ b/src/tests/angle_end2end_tests_expectations.txt
@@ -2723,6 +2723,8 @@ 500930576 OPENGL : ReadPixelsTest.LargeTexture/* = SKIP // Crashes inside AMD Metal driver when copying the texture -> buffer 500930576 METAL AMD : ReadPixelsTest.LargeTexture/* = SKIP +// Fails test on Metal +499091328 MAC AMD METAL : Texture2DArrayTestES3.ReformatStagedBufferUpdatesLayerCountOOB/* = SKIP // Timeout on Intel 509829264 VULKAN WIN INTEL : SimpleStateChangeTestES31.TransformFeedbackThenReadWithCompute/* = SKIP
diff --git a/src/tests/gl_tests/TextureTest.cpp b/src/tests/gl_tests/TextureTest.cpp index d84daf1..821167c 100644 --- a/src/tests/gl_tests/TextureTest.cpp +++ b/src/tests/gl_tests/TextureTest.cpp
@@ -9205,6 +9205,50 @@ ASSERT_GL_NO_ERROR(); } +// Test that OOB read doesn't occur as a result of reformatStagedBufferUpdates using +// the depth instead of the layer count for 2D arrays. This would only occur on Vulkan drivers +// where VK_FORMAT_R4G4B4A4_UNORM_PACK16 is sample-only (no COLOR_ATTACHMENT support). This forces +// ANGLE to pick R8G8B8A8_UNORM as the renderable fallback for GL_RGBA4, resulting in +// reformatStagedBufferUpdates getting called. See crbug.com/499091328. +// +// Dimensions chosen so that the *single-layer* reformatted RGBA8 staging buffer +// (W * H * 4 bytes) exceeds ANGLE's kMaxBufferSizeForSuballocation (8 MiB). That +// forces a dedicated VkBuffer of exactly W*H*4 bytes; vkCmdCopyBufferToImage with +// layerCount=D, which used to then read (D-1)*W*H*4 bytes past the end of that VkBuffer. +TEST_P(Texture2DArrayTestES3, ReformatStagedBufferUpdatesLayerCountOOB) +{ + const int W = 1500; + const int H = 1500; + const int D = 4; + + GLTexture tex; + glBindTexture(GL_TEXTURE_2D_ARRAY, tex); + glTexImage3D(GL_TEXTURE_2D_ARRAY, 0, GL_RGBA4, W, H, D, 0, GL_RGBA, GL_UNSIGNED_SHORT_4_4_4_4, + nullptr); + ASSERT_GL_NO_ERROR(); + + // Stage a buffer update covering all D layers. stageSubresourceUpdateImpl() + // sets copy.imageExtent.depth = 1 and copy.imageSubresource.layerCount = D. + // The update is staged with formatID = R4G4B4A4_UNORM (sample-only format). + std::vector<uint16_t> pixels(W * H * D, 0xF0FF); + glTexSubImage3D(GL_TEXTURE_2D_ARRAY, 0, 0, 0, 0, W, H, D, GL_RGBA, GL_UNSIGNED_SHORT_4_4_4_4, + pixels.data()); + ASSERT_GL_NO_ERROR(); + + // Bind the texture as a framebuffer attachment. This sets hasBeenBoundAsAttachment(); the next + // syncState() will call ensureRenderable() -> reformatStagedBufferUpdates(). + GLFramebuffer fb; + glBindFramebuffer(GL_FRAMEBUFFER, fb); + glFramebufferTextureLayer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, tex, 0, D - 1); + ASSERT_GL_NO_ERROR(); + ASSERT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER); + + // Force flush of the now-reformatted, and once undersized (before bug fix), staged update via + // vkCmdCopyBufferToImage. readPixels syncs the framebuffer, which ensures the attached texture + // is initialized -> flushStagedUpdates -> potential GPU OOB read. + EXPECT_PIXEL_RECT_EQ(0, 0, W, H, GLColor::magenta); +} + // Test shadow sampler and regular non-shadow sampler coexisting in the same shader. // This test is needed especially to confirm that sampler registers get assigned correctly on // the HLSL backend even when there's a mix of different HLSL sampler and texture types.