Browse Source

chore: cherry-pick ca32852e4f from angle (#33222)

Co-authored-by: Electron Bot <[email protected]>
Pedro Pontes 3 years ago
parent
commit
2f28e10543

+ 2 - 0
patches/angle/.patches

@@ -8,4 +8,6 @@ m96_validate_samplerformat.patch
 m98_vulkan_fix_vkcmdresolveimage_extents.patch
 m98_vulkan_fix_vkcmdresolveimage_offsets.patch
 cherry-pick-49e8ff16f1fe.patch
+m99_vulkan_prevent_out_of_bounds_read_in_divisor_emulation_path.patch
+m99_vulkan_streamvertexdatawithdivisor_write_beyond_buffer_boundary.patch
 m98_protect_against_deleting_a_current_xfb_buffer.patch

+ 303 - 0
patches/angle/m99_vulkan_prevent_out_of_bounds_read_in_divisor_emulation_path.patch

@@ -0,0 +1,303 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Roman Lavrov <[email protected]>
+Date: Tue, 18 Jan 2022 20:05:55 +0000
+Subject: M99: Vulkan: Prevent out of bounds read in divisor emulation path.
+
+Split the replicated part of StreamVertexData out to
+StreamVertexDataWithDivisor, there is only a partial argument
+overlap between the two.
+
+Bug: chromium:1285885
+Change-Id: Ibf6ab3efc6b12b430b1d391c6ae61bd9668b4407
+Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3398816
+Reviewed-by: Jamie Madill <[email protected]>
+Reviewed-by: Shahbaz Youssefi <[email protected]>
+Commit-Queue: Roman Lavrov <[email protected]>
+(cherry picked from commit 5f0badf4541ba52659c937cfe9190d3735a76c10)
+Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3461414
+
+diff --git a/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp b/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
+index ed416508cab06797a8c4d52e7ab982d538f57e3c..1ca486af3ff004e11aea82c391b4c504b569096c 100644
+--- a/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
++++ b/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
+@@ -81,34 +81,62 @@ angle::Result StreamVertexData(ContextVk *contextVk,
+                                size_t destOffset,
+                                size_t vertexCount,
+                                size_t sourceStride,
+-                               size_t destStride,
+                                VertexCopyFunction vertexLoadFunction,
+                                vk::BufferHelper **bufferOut,
+-                               VkDeviceSize *bufferOffsetOut,
+-                               uint32_t replicateCount)
++                               VkDeviceSize *bufferOffsetOut)
+ {
+     uint8_t *dst = nullptr;
+     ANGLE_TRY(dynamicBuffer->allocate(contextVk, bytesToAllocate, &dst, nullptr, bufferOffsetOut,
+                                       nullptr));
+     *bufferOut = dynamicBuffer->getCurrentBuffer();
+     dst += destOffset;
+-    if (replicateCount == 1)
++    vertexLoadFunction(sourceData, sourceStride, vertexCount, dst);
++
++    ANGLE_TRY(dynamicBuffer->flush(contextVk));
++    return angle::Result::Continue;
++}
++
++angle::Result StreamVertexDataWithDivisor(ContextVk *contextVk,
++                                          vk::DynamicBuffer *dynamicBuffer,
++                                          const uint8_t *sourceData,
++                                          size_t bytesToAllocate,
++                                          size_t sourceStride,
++                                          size_t destStride,
++                                          VertexCopyFunction vertexLoadFunction,
++                                          vk::BufferHelper **bufferOut,
++                                          VkDeviceSize *bufferOffsetOut,
++                                          uint32_t divisor,
++                                          size_t numSrcVertices)
++{
++    uint8_t *dst = nullptr;
++    ANGLE_TRY(dynamicBuffer->allocate(contextVk, bytesToAllocate, &dst, nullptr, bufferOffsetOut,
++                                      nullptr));
++    *bufferOut = dynamicBuffer->getCurrentBuffer();
++
++    // Each source vertex is used `divisor` times before advancing. Clamp to avoid OOB reads.
++    size_t clampedSize = std::min(numSrcVertices * destStride * divisor, bytesToAllocate);
++
++    ASSERT(clampedSize % destStride == 0);
++    ASSERT(divisor > 0);
++
++    uint32_t sourceVertexUseCount = 0;
++    for (size_t dataCopied = 0; dataCopied < clampedSize; dataCopied += destStride, dst += destStride)
+     {
+-        vertexLoadFunction(sourceData, sourceStride, vertexCount, dst);
++        vertexLoadFunction(sourceData, sourceStride, 1, dst);
++        sourceVertexUseCount++;
++        if (sourceVertexUseCount == divisor)
++        {
++            sourceData += sourceStride;
++            sourceVertexUseCount = 0;
++        }
+     }
+-    else
++
++    // Satisfy robustness constraints (only if extension enabled)
++    if (contextVk->getExtensions().robustness)
+     {
+-        ASSERT(replicateCount > 1);
+-        uint32_t sourceRemainingCount = replicateCount - 1;
+-        for (size_t dataCopied = 0; dataCopied < bytesToAllocate;
+-             dataCopied += destStride, dst += destStride, sourceRemainingCount--)
++        if (clampedSize < bytesToAllocate)
+         {
+-            vertexLoadFunction(sourceData, sourceStride, 1, dst);
+-            if (sourceRemainingCount == 0)
+-            {
+-                sourceData += sourceStride;
+-                sourceRemainingCount = replicateCount;
+-            }
++            memset(dst + clampedSize, 0, bytesToAllocate - clampedSize);
+         }
+     }
+ 
+@@ -124,6 +152,7 @@ size_t GetVertexCount(BufferVk *srcBuffer, const gl::VertexBinding &binding, uin
+         return 0;
+ 
+     // Count the last vertex.  It may occupy less than a full stride.
++    // This is also correct if stride happens to be less than srcFormatSize.
+     size_t numVertices = 1;
+     bytes -= srcFormatSize;
+ 
+@@ -452,8 +481,8 @@ angle::Result VertexArrayVk::convertVertexBufferCPU(ContextVk *contextVk,
+     ASSERT(GetVertexInputAlignment(vertexFormat, compressed) <= vk::kVertexBufferAlignment);
+     ANGLE_TRY(StreamVertexData(
+         contextVk, &conversion->data, srcBytes, numVertices * dstFormatSize, 0, numVertices,
+-        binding.getStride(), srcFormatSize, vertexFormat.getVertexLoadFunction(compressed),
+-        &mCurrentArrayBuffers[attribIndex], &conversion->lastAllocationOffset, 1));
++        binding.getStride(), vertexFormat.getVertexLoadFunction(compressed),
++        &mCurrentArrayBuffers[attribIndex], &conversion->lastAllocationOffset));
+     ANGLE_TRY(srcBuffer->unmapImpl(contextVk));
+ 
+     ASSERT(conversion->dirty);
+@@ -808,28 +837,41 @@ angle::Result VertexArrayVk::updateStreamedAttribs(const gl::Context *context,
+             // Instanced attrib
+             if (divisor > renderer->getMaxVertexAttribDivisor())
+             {
+-                // Emulated attrib
+-                BufferVk *bufferVk = nullptr;
++                // Divisor will be set to 1 & so update buffer to have 1 attrib per instance
++                size_t bytesToAllocate = instanceCount * stride;
++
+                 if (binding.getBuffer().get() != nullptr)
+                 {
+                     // Map buffer to expand attribs for divisor emulation
+-                    bufferVk      = vk::GetImpl(binding.getBuffer().get());
+-                    void *buffSrc = nullptr;
++                    BufferVk *bufferVk      = vk::GetImpl(binding.getBuffer().get());
++                    void *buffSrc           = nullptr;
+                     ANGLE_TRY(bufferVk->mapImpl(contextVk, &buffSrc));
+                     src = reinterpret_cast<const uint8_t *>(buffSrc) + binding.getOffset();
+-                }
+-                // Divisor will be set to 1 & so update buffer to have 1 attrib per instance
+-                size_t bytesToAllocate = instanceCount * stride;
+ 
+-                ANGLE_TRY(StreamVertexData(contextVk, &mDynamicVertexData, src, bytesToAllocate, 0,
+-                                           instanceCount, binding.getStride(), stride,
+-                                           vertexFormat.vertexLoadFunction,
+-                                           &mCurrentArrayBuffers[attribIndex],
+-                                           &mCurrentArrayBufferOffsets[attribIndex], divisor));
+-                if (bufferVk)
+-                {
++                    uint32_t srcAttributeSize =
++                        static_cast<uint32_t>(ComputeVertexAttributeTypeSize(attrib));
++
++                    size_t numVertices = GetVertexCount(bufferVk, binding, srcAttributeSize);
++
++                    ANGLE_TRY(StreamVertexDataWithDivisor(
++                        contextVk, &mDynamicVertexData, src, bytesToAllocate, binding.getStride(),
++                        stride, vertexFormat.vertexLoadFunction,
++                        &mCurrentArrayBuffers[attribIndex],
++                        &mCurrentArrayBufferOffsets[attribIndex], divisor,
++                        numVertices));
++
+                     ANGLE_TRY(bufferVk->unmapImpl(contextVk));
+                 }
++                else
++                {
++                    size_t numVertices = instanceCount;
++                    ANGLE_TRY(StreamVertexDataWithDivisor(
++                        contextVk, &mDynamicVertexData, src, bytesToAllocate, binding.getStride(),
++                        stride, vertexFormat.vertexLoadFunction,
++                        &mCurrentArrayBuffers[attribIndex],
++                        &mCurrentArrayBufferOffsets[attribIndex], divisor,
++                        numVertices));
++                }
+             }
+             else
+             {
+@@ -838,10 +880,10 @@ angle::Result VertexArrayVk::updateStreamedAttribs(const gl::Context *context,
+                 size_t bytesToAllocate = count * stride;
+ 
+                 ANGLE_TRY(StreamVertexData(contextVk, &mDynamicVertexData, src, bytesToAllocate, 0,
+-                                           count, binding.getStride(), stride,
++                                           count, binding.getStride(),
+                                            vertexFormat.vertexLoadFunction,
+                                            &mCurrentArrayBuffers[attribIndex],
+-                                           &mCurrentArrayBufferOffsets[attribIndex], 1));
++                                           &mCurrentArrayBufferOffsets[attribIndex]));
+             }
+         }
+         else
+@@ -856,8 +898,8 @@ angle::Result VertexArrayVk::updateStreamedAttribs(const gl::Context *context,
+ 
+             ANGLE_TRY(StreamVertexData(
+                 contextVk, &mDynamicVertexData, src, bytesToAllocate, destOffset, vertexCount,
+-                binding.getStride(), stride, vertexFormat.vertexLoadFunction,
+-                &mCurrentArrayBuffers[attribIndex], &mCurrentArrayBufferOffsets[attribIndex], 1));
++                binding.getStride(), vertexFormat.vertexLoadFunction,
++                &mCurrentArrayBuffers[attribIndex], &mCurrentArrayBufferOffsets[attribIndex]));
+         }
+ 
+         mCurrentArrayBufferHandles[attribIndex] =
+diff --git a/src/tests/gl_tests/RobustBufferAccessBehaviorTest.cpp b/src/tests/gl_tests/RobustBufferAccessBehaviorTest.cpp
+index 08917a2de3385eb853bced1dd576aff46f34f709..db708b0a2a3ce90a370b43786af6884b2f992bef 100644
+--- a/src/tests/gl_tests/RobustBufferAccessBehaviorTest.cpp
++++ b/src/tests/gl_tests/RobustBufferAccessBehaviorTest.cpp
+@@ -564,6 +564,98 @@ TEST_P(RobustBufferAccessBehaviorTest, DynamicBuffer)
+     }
+ }
+ 
++// Tests out of bounds read by divisor emulation due to a user-provided offset.
++// Adapted from https://crbug.com/1285885.
++TEST_P(RobustBufferAccessBehaviorTest, IndexOutOfBounds)
++{
++    ANGLE_SKIP_TEST_IF(!initExtension());
++
++    constexpr char kVS[] = R"(precision highp float;
++attribute vec4 a_position;
++void main(void) {
++   gl_Position = a_position;
++})";
++
++    constexpr char kFS[] = R"(precision highp float;
++uniform sampler2D oTexture;
++uniform float oColor[3];
++void main(void) {
++   gl_FragData[0] = texture2DProj(oTexture, vec3(0.1,0.1,0.1));
++})";
++
++    GLfloat singleFloat = 1.0f;
++
++    GLBuffer buf;
++    glBindBuffer(GL_ARRAY_BUFFER, buf);
++    glBufferData(GL_ARRAY_BUFFER, 4, &singleFloat, GL_STATIC_DRAW);
++
++    ANGLE_GL_PROGRAM(program, kVS, kFS);
++    glBindAttribLocation(program, 0, "a_position");
++    glLinkProgram(program);
++    ASSERT_TRUE(CheckLinkStatusAndReturnProgram(program, true));
++
++    glEnableVertexAttribArray(0);
++
++    // Trying to exceed renderer->getMaxVertexAttribDivisor()
++    GLuint constexpr kDivisor = 4096;
++    glVertexAttribDivisor(0, kDivisor);
++
++    size_t outOfBoundsOffset = 0x50000000;
++    glVertexAttribPointer(0, 1, GL_FLOAT, false, 8, reinterpret_cast<void *>(outOfBoundsOffset));
++
++    glUseProgram(program);
++
++    glDrawArrays(GL_TRIANGLES, 0, 32);
++
++    // No assertions, just checking for crashes.
++}
++
++// Similar to the test above but index is first within bounds then goes out of bounds.
++TEST_P(RobustBufferAccessBehaviorTest, IndexGoingOutOfBounds)
++{
++    ANGLE_SKIP_TEST_IF(!initExtension());
++
++    constexpr char kVS[] = R"(precision highp float;
++attribute vec4 a_position;
++void main(void) {
++   gl_Position = a_position;
++})";
++
++    constexpr char kFS[] = R"(precision highp float;
++uniform sampler2D oTexture;
++uniform float oColor[3];
++void main(void) {
++   gl_FragData[0] = texture2DProj(oTexture, vec3(0.1,0.1,0.1));
++})";
++
++    GLBuffer buf;
++    glBindBuffer(GL_ARRAY_BUFFER, buf);
++    std::array<GLfloat, 2> buffer = {{0.2f, 0.2f}};
++    glBufferData(GL_ARRAY_BUFFER, sizeof(GLfloat) * buffer.size(), buffer.data(), GL_STATIC_DRAW);
++
++    ANGLE_GL_PROGRAM(program, kVS, kFS);
++    glBindAttribLocation(program, 0, "a_position");
++    glLinkProgram(program);
++    ASSERT_TRUE(CheckLinkStatusAndReturnProgram(program, true));
++
++    glEnableVertexAttribArray(0);
++
++    // Trying to exceed renderer->getMaxVertexAttribDivisor()
++    GLuint constexpr kDivisor = 4096;
++    glVertexAttribDivisor(0, kDivisor);
++
++    // 6 bytes remaining in the buffer from offset so only a single vertex can be read
++    glVertexAttribPointer(0, 1, GL_FLOAT, false, 8, reinterpret_cast<void *>(2));
++
++    glUseProgram(program);
++
++    // Each vertex is read `kDivisor` times so the last read goes out of bounds
++    GLsizei instanceCount = kDivisor + 1;
++    glDrawArraysInstanced(GL_TRIANGLES, 0, 32, instanceCount);
++
++    // No assertions, just checking for crashes.
++}
++
+ ANGLE_INSTANTIATE_TEST_ES2_AND_ES3_AND_ES31(RobustBufferAccessBehaviorTest);
+ 
+ }  // namespace

+ 51 - 0
patches/angle/m99_vulkan_streamvertexdatawithdivisor_write_beyond_buffer_boundary.patch

@@ -0,0 +1,51 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Charlie Lao <[email protected]>
+Date: Mon, 7 Feb 2022 15:08:15 -0800
+Subject: M99: Vulkan: StreamVertexDataWithDivisor write beyond buffer boundary
+
+StreamVertexDataWithDivisor() function is advancing dst with dstStride,
+but then later on it is treating dst as if it never advanced, thus
+result in write out of buffer boundary. This was hidden by VMA's memory
+suballocation, which means it may result in some rendering artifacts.
+
+Bug: angleproject:6990
+Change-Id: Ic91e917cedd247dfe85b12a69ae26b21b7a4e67a
+Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3445528
+Reviewed-by: Roman Lavrov <[email protected]>
+Reviewed-by: Jamie Madill <[email protected]>
+Commit-Queue: Charlie Lao <[email protected]>
+(cherry picked from commit 5204587698099207ce8ae70779ef44ffae877996)
+Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3461417
+Reviewed-by: Charlie Lao <[email protected]>
+Commit-Queue: Roman Lavrov <[email protected]>
+
+diff --git a/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp b/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
+index 1ca486af3ff004e11aea82c391b4c504b569096c..93378c4b24495872405fc06ea01e15254229ab63 100644
+--- a/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
++++ b/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
+@@ -120,7 +120,7 @@ angle::Result StreamVertexDataWithDivisor(ContextVk *contextVk,
+     ASSERT(divisor > 0);
+ 
+     uint32_t sourceVertexUseCount = 0;
+-    for (size_t dataCopied = 0; dataCopied < clampedSize; dataCopied += destStride, dst += destStride)
++    for (size_t dataCopied = 0; dataCopied < clampedSize; dataCopied += destStride)
+     {
+         vertexLoadFunction(sourceData, sourceStride, 1, dst);
+         sourceVertexUseCount++;
+@@ -129,6 +129,7 @@ angle::Result StreamVertexDataWithDivisor(ContextVk *contextVk,
+             sourceData += sourceStride;
+             sourceVertexUseCount = 0;
+         }
++        dst += destStride;
+     }
+ 
+     // Satisfy robustness constraints (only if extension enabled)
+@@ -136,7 +137,7 @@ angle::Result StreamVertexDataWithDivisor(ContextVk *contextVk,
+     {
+         if (clampedSize < bytesToAllocate)
+         {
+-            memset(dst + clampedSize, 0, bytesToAllocate - clampedSize);
++            memset(dst, 0, bytesToAllocate - clampedSize);
+         }
+     }
+