Fix integer overflow in vertex buffer access robustness check
The spec states that "vertex input attributes are considered out of
bounds if the offset of the attribute in the bound vertex buffer range
plus the size of the attribute is greater than ...". This addition can
cause integer overflow and make us fail to detect out of bounds
accesses.
Note there aren't x86 SIMD instructions for 32-bit arithmetic with
saturation. But we can cheaply prevent this overflow by first clamping
the offset to the range, and then doing the addition. Even for an
attribute size of 1, we would detect the out-of-bounds access.
Note that this relies on the range itself not being close to 4 GiB. We
currently limit all device memory allocations to 1 GiB, which ought to
be enough for anyone.
Bug: b/195684837
Bug: chromium:1234701
Change-Id: Ie8ec0859566e3aafb7a592bc4e92119a79016eb1
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/56271
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
diff --git a/src/Pipeline/VertexRoutine.cpp b/src/Pipeline/VertexRoutine.cpp
index 8905fa8..aff1bc4 100644
--- a/src/Pipeline/VertexRoutine.cpp
+++ b/src/Pipeline/VertexRoutine.cpp
@@ -188,13 +188,19 @@
UInt4 zero(0);
if(robustBufferAccess)
{
- // TODO(b/141124876): Optimize for wide-vector gather operations.
+ // Prevent integer overflow on the addition below.
+ offsets = Min(offsets, UInt4(robustnessSize));
+
+ // "vertex input attributes are considered out of bounds if the offset of the attribute
+ // in the bound vertex buffer range plus the size of the attribute is greater than ..."
UInt4 limits = offsets + UInt4(format.bytes());
+
Pointer<Byte> zeroSource = As<Pointer<Byte>>(&zero);
- source0 = IfThenElse(limits.x <= robustnessSize, source0, zeroSource);
- source1 = IfThenElse(limits.y <= robustnessSize, source1, zeroSource);
- source2 = IfThenElse(limits.z <= robustnessSize, source2, zeroSource);
- source3 = IfThenElse(limits.w <= robustnessSize, source3, zeroSource);
+ // TODO(b/141124876): Optimize for wide-vector gather operations.
+ source0 = IfThenElse(limits.x > robustnessSize, zeroSource, source0);
+ source1 = IfThenElse(limits.y > robustnessSize, zeroSource, source1);
+ source2 = IfThenElse(limits.z > robustnessSize, zeroSource, source2);
+ source3 = IfThenElse(limits.w > robustnessSize, zeroSource, source3);
}
int componentCount = format.componentCount();
diff --git a/src/Vulkan/VkDescriptorSetLayout.cpp b/src/Vulkan/VkDescriptorSetLayout.cpp
index f18a3a7..7a1079f 100644
--- a/src/Vulkan/VkDescriptorSetLayout.cpp
+++ b/src/Vulkan/VkDescriptorSetLayout.cpp
@@ -481,6 +481,9 @@
auto buffer = vk::Cast(update->buffer);
bufferDescriptor[i].ptr = buffer->getOffsetPointer(update->offset);
bufferDescriptor[i].sizeInBytes = static_cast<int>((update->range == VK_WHOLE_SIZE) ? buffer->getSize() - update->offset : update->range);
+
+ // TODO(b/195684837): The spec states that "vertexBufferRangeSize is the byte size of the memory
+ // range bound to the vertex buffer binding", while the code below uses the full size of the buffer.
bufferDescriptor[i].robustnessSize = static_cast<int>(buffer->getSize() - update->offset);
}
}
