Skip to content

Commit

Permalink
chore: cherry-pick ca32852e4f from angle
Browse files Browse the repository at this point in the history
  • Loading branch information
@ppontes
ppontes committed Mar 10, 2022
1 parent 8519ba8 commit 99fccc9
Show file tree
Hide file tree
Showing 3 changed files with 356 additions and 0 deletions.
2 changes: 2 additions & 0 deletions patches/angle/.patches
View file
Expand Up @@ -7,3 +7,5 @@ vangle_change_the_default_vulkan_device_choose_logic.patch
m96_validate_samplerformat.patch
m98_vulkan_fix_vkcmdresolveimage_extents.patch
m98_vulkan_fix_vkcmdresolveimage_offsets.patch
m99_vulkan_prevent_out_of_bounds_read_in_divisor_emulation_path.patch
m99_vulkan_streamvertexdatawithdivisor_write_beyond_buffer_boundary.patch
303 changes: 303 additions & 0 deletions patches/angle/m99_vulkan_prevent_out_of_bounds_read_in_divisor_emulation_path.patch
View file
@@ -0,0 +1,303 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Roman Lavrov <romanl@google.com>
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 <jmadill@chromium.org>
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Commit-Queue: Roman Lavrov <romanl@google.com>
(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 changes: 51 additions & 0 deletions patches/angle/m99_vulkan_streamvertexdatawithdivisor_write_beyond_buffer_boundary.patch
View file
@@ -0,0 +1,51 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Charlie Lao <cclao@google.com>
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 <romanl@google.com>
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Commit-Queue: Charlie Lao <cclao@google.com>
(cherry picked from commit 5204587698099207ce8ae70779ef44ffae877996)
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3461417
Reviewed-by: Charlie Lao <cclao@google.com>
Commit-Queue: Roman Lavrov <romanl@google.com>

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);
}
}

0 comments on commit 99fccc9

Please sign in to comment.