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