gpu: Check index buffer contents in pre-draw validation

This involves splitting up draw.vert into separate shaders for non-indexed indirect, indexed (both indirect and direct) and mesh draw calls. Fixes KhronosGroup#2492
arno-lunarg · Aug 19, 2024 · f446338 · f446338
1 parent 8d902c7
commit f446338
Show file tree

Hide file tree

Showing 31 changed files with 2,621 additions and 616 deletions.
diff --git a/BUILD.gn b/BUILD.gn
@@ -335,8 +335,12 @@ vvl_sources = [
   "layers/vulkan/generated/cmd_validation_copy_buffer_to_image_comp.h",
   "layers/vulkan/generated/cmd_validation_dispatch_comp.cpp",
   "layers/vulkan/generated/cmd_validation_dispatch_comp.h",
-  "layers/vulkan/generated/cmd_validation_draw_vert.cpp",
-  "layers/vulkan/generated/cmd_validation_draw_vert.h",
+  "layers/vulkan/generated/cmd_validation_indexed_draw_vert.h",
+  "layers/vulkan/generated/cmd_validation_indexed_draw_vert.cpp",
+  "layers/vulkan/generated/cmd_validation_indirect_draw_vert.h",
+  "layers/vulkan/generated/cmd_validation_indirect_draw_vert.cpp",
+  "layers/vulkan/generated/cmd_validation_mesh_draw_vert.h",
+  "layers/vulkan/generated/cmd_validation_mesh_draw_vert.cpp",
   "layers/vulkan/generated/cmd_validation_trace_rays_rgen.cpp",
   "layers/vulkan/generated/cmd_validation_trace_rays_rgen.h",
   "layers/vulkan/generated/command_validation.cpp",

diff --git a/layers/CMakeLists.txt b/layers/CMakeLists.txt
@@ -213,8 +213,12 @@ target_sources(vvl PRIVATE
     ${API_TYPE}/generated/cmd_validation_copy_buffer_to_image_comp.cpp
     ${API_TYPE}/generated/cmd_validation_dispatch_comp.h
     ${API_TYPE}/generated/cmd_validation_dispatch_comp.cpp
-    ${API_TYPE}/generated/cmd_validation_draw_vert.h
-    ${API_TYPE}/generated/cmd_validation_draw_vert.cpp
+    ${API_TYPE}/generated/cmd_validation_indexed_draw_vert.h
+    ${API_TYPE}/generated/cmd_validation_indexed_draw_vert.cpp
+    ${API_TYPE}/generated/cmd_validation_indirect_draw_vert.h
+    ${API_TYPE}/generated/cmd_validation_indirect_draw_vert.cpp
+    ${API_TYPE}/generated/cmd_validation_mesh_draw_vert.h
+    ${API_TYPE}/generated/cmd_validation_mesh_draw_vert.cpp
     ${API_TYPE}/generated/cmd_validation_trace_rays_rgen.h
     ${API_TYPE}/generated/cmd_validation_trace_rays_rgen.cpp
     gpu/core/gpu_settings.h

diff --git a/layers/gpu/cmd_validation/gpuav_draw.cpp b/layers/gpu/cmd_validation/gpuav_draw.cpp
diff --git a/layers/gpu/cmd_validation/gpuav_draw.h b/layers/gpu/cmd_validation/gpuav_draw.h
@@ -21,13 +21,25 @@
 
 struct Location;
 
+namespace vvl {
+class Buffer;
+}
+
 namespace gpuav {
 class Validator;
 
 void DestroyRenderPassMappedResources(Validator &gpuav, VkRenderPass render_pass);
 
-void InsertIndirectDrawValidation(Validator &gpuav, const Location &loc, CommandBuffer &cb_state, VkBuffer indirect_buffer,
-                                  VkDeviceSize indirect_offset, uint32_t draw_count, VkBuffer count_buffer,
-                                  VkDeviceSize count_buffer_offset, uint32_t stride);
+void InsertIndirectDrawValidation(Validator &gpuav, const Location &loc, CommandBuffer &cb_state,
+                                  vvl::Buffer &indirect_buffer_state, VkDeviceSize indirect_offset, uint32_t draw_count,
+                                  VkBuffer count_buffer, VkDeviceSize count_buffer_offset, uint32_t stride);
+
+void InsertIndexedDrawValidation(Validator &gpuav, const Location &loc, CommandBuffer &cb_state, VkBuffer indirect_buffer,
+                                 VkDeviceSize indirect_offset, uint32_t draw_count, VkBuffer count_buffer,
+                                 VkDeviceSize count_buffer_offset, uint32_t stride, uint32_t first_index, uint32_t index_count);
+
+void InsertIndirectMeshDrawValidation(Validator &gpuav, const Location &loc, CommandBuffer &cb_state,
+                                      vvl::Buffer &indirect_buffer_state, VkDeviceSize indirect_offset, uint32_t draw_count,
+                                      VkBuffer count_buffer, VkDeviceSize count_buffer_offset, uint32_t stride);
 
 }  // namespace gpuav
diff --git a/layers/gpu/core/gpuav.h b/layers/gpu/core/gpuav.h
@@ -321,7 +321,7 @@ class Validator : public gpu::GpuShaderInstrumentor {
 
   public:
     std::optional<DescriptorHeap> desc_heap_{};  // optional only to defer construction
-    gpu::SharedResourcesManager shared_resources_manager;
+    gpu::SharedResourcesCache shared_resources_manager;
 
   private:
     std::string instrumented_shader_cache_path_{};

diff --git a/layers/gpu/core/gpuav_record.cpp b/layers/gpu/core/gpuav_record.cpp
@@ -48,10 +48,15 @@ void Validator::PreCallRecordCreateBuffer(VkDevice device, const VkBufferCreateI
 
     // Indirect buffers will require validation shader to bind the indirect buffers as a storage buffer.
     if (gpuav_settings.IsBufferValidationEnabled() &&
-        chassis_state.modified_create_info.usage & VK_BUFFER_USAGE_INDIRECT_BUFFER_BIT) {
+        (chassis_state.modified_create_info.usage & (VK_BUFFER_USAGE_INDIRECT_BUFFER_BIT | VK_BUFFER_USAGE_INDEX_BUFFER_BIT))) {
         chassis_state.modified_create_info.usage |= VK_BUFFER_USAGE_STORAGE_BUFFER_BIT;
     }
 
+    // Align index buffer size to 4: validation shader reads DWORDS
+    if (gpuav_settings.IsBufferValidationEnabled()) {
+        chassis_state.modified_create_info.size = Align<VkDeviceSize>(chassis_state.modified_create_info.size, 4);
+    }
+
     BaseClass::PreCallRecordCreateBuffer(device, pCreateInfo, pAllocator, pBuffer, record_obj, chassis_state);
 }
 
@@ -337,6 +342,8 @@ void Validator::PreCallRecordCmdDrawIndexed(VkCommandBuffer commandBuffer, uint3
         InternalError(commandBuffer, record_obj.location, "Unrecognized command buffer.");
         return;
     }
+    InsertIndexedDrawValidation(*this, record_obj.location, *cb_state, VK_NULL_HANDLE, 0, 0, VK_NULL_HANDLE, 0, 0, firstIndex,
+                                indexCount);
     SetupShaderInstrumentationResources(*this, *cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, record_obj.location);
 }
 
@@ -367,7 +374,14 @@ void Validator::PreCallRecordCmdDrawIndirect(VkCommandBuffer commandBuffer, VkBu
         return;
     }
 
-    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, count, VK_NULL_HANDLE, 0, stride);
+    auto indirect_buffer_state = Get<vvl::Buffer>(buffer);
+    if (!indirect_buffer_state) {
+        InternalError(LogObjectList(commandBuffer, buffer), record_obj.location, "buffer must be a valid VkBuffer handle");
+        return;
+    }
+
+    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, *indirect_buffer_state, offset, count, VK_NULL_HANDLE, 0,
+                                 stride);
     SetupShaderInstrumentationResources(*this, *cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, record_obj.location);
 }
 
@@ -381,7 +395,7 @@ void Validator::PreCallRecordCmdDrawIndexedIndirect(VkCommandBuffer commandBuffe
         return;
     }
 
-    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, count, VK_NULL_HANDLE, 0, stride);
+    InsertIndexedDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, count, VK_NULL_HANDLE, 0, stride, 0, 0);
     SetupShaderInstrumentationResources(*this, *cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, record_obj.location);
 }
 
@@ -403,7 +417,14 @@ void Validator::PreCallRecordCmdDrawIndirectCount(VkCommandBuffer commandBuffer,
         InternalError(commandBuffer, record_obj.location, "Unrecognized command buffer.");
         return;
     }
-    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, maxDrawCount, countBuffer,
+
+    auto indirect_buffer_state = Get<vvl::Buffer>(buffer);
+    if (!indirect_buffer_state) {
+        InternalError(LogObjectList(commandBuffer, buffer), record_obj.location, "buffer must be a valid VkBuffer handle");
+        return;
+    }
+
+    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, *indirect_buffer_state, offset, maxDrawCount, countBuffer,
                                  countBufferOffset, stride);
     SetupShaderInstrumentationResources(*this, *cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, record_obj.location);
 }
@@ -441,8 +462,8 @@ void Validator::PreCallRecordCmdDrawIndexedIndirectCount(VkCommandBuffer command
         InternalError(commandBuffer, record_obj.location, "Unrecognized command buffer.");
         return;
     }
-    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, maxDrawCount, countBuffer,
-                                 countBufferOffset, stride);
+    InsertIndexedDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, maxDrawCount, countBuffer, countBufferOffset,
+                                stride, 0, 0);
     SetupShaderInstrumentationResources(*this, *cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, record_obj.location);
 }
 
@@ -466,7 +487,13 @@ void Validator::PreCallRecordCmdDrawMeshTasksIndirectNV(VkCommandBuffer commandB
         InternalError(commandBuffer, record_obj.location, "Unrecognized command buffer.");
         return;
     }
-    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, drawCount, VK_NULL_HANDLE, 0, stride);
+    auto indirect_buffer_state = Get<vvl::Buffer>(buffer);
+    if (!indirect_buffer_state) {
+        InternalError(LogObjectList(commandBuffer, buffer), record_obj.location, "buffer must be a valid VkBuffer handle");
+        return;
+    }
+    InsertIndirectMeshDrawValidation(*this, record_obj.location, *cb_state, *indirect_buffer_state, offset, drawCount,
+                                     VK_NULL_HANDLE, 0, stride);
     SetupShaderInstrumentationResources(*this, *cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, record_obj.location);
 }
 
@@ -482,8 +509,13 @@ void Validator::PreCallRecordCmdDrawMeshTasksIndirectCountNV(VkCommandBuffer com
         InternalError(commandBuffer, record_obj.location, "Unrecognized command buffer.");
         return;
     }
-    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, maxDrawCount, countBuffer,
-                                 countBufferOffset, stride);
+    auto indirect_buffer_state = Get<vvl::Buffer>(buffer);
+    if (!indirect_buffer_state) {
+        InternalError(LogObjectList(commandBuffer, buffer), record_obj.location, "buffer must be a valid VkBuffer handle");
+        return;
+    }
+    InsertIndirectMeshDrawValidation(*this, record_obj.location, *cb_state, *indirect_buffer_state, offset, maxDrawCount,
+                                     countBuffer, countBufferOffset, stride);
     SetupShaderInstrumentationResources(*this, *cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, record_obj.location);
 }
 
@@ -507,7 +539,13 @@ void Validator::PreCallRecordCmdDrawMeshTasksIndirectEXT(VkCommandBuffer command
         InternalError(commandBuffer, record_obj.location, "Unrecognized command buffer.");
         return;
     }
-    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, drawCount, VK_NULL_HANDLE, 0, stride);
+    auto indirect_buffer_state = Get<vvl::Buffer>(buffer);
+    if (!indirect_buffer_state) {
+        InternalError(LogObjectList(commandBuffer, buffer), record_obj.location, "buffer must be a valid VkBuffer handle");
+        return;
+    }
+    InsertIndirectMeshDrawValidation(*this, record_obj.location, *cb_state, *indirect_buffer_state, offset, drawCount,
+                                     VK_NULL_HANDLE, 0, stride);
     SetupShaderInstrumentationResources(*this, *cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, record_obj.location);
 }
 
@@ -523,8 +561,13 @@ void Validator::PreCallRecordCmdDrawMeshTasksIndirectCountEXT(VkCommandBuffer co
         InternalError(commandBuffer, record_obj.location, "Unrecognized command buffer.");
         return;
     }
-    InsertIndirectDrawValidation(*this, record_obj.location, *cb_state, buffer, offset, maxDrawCount, countBuffer,
-                                 countBufferOffset, stride);
+    auto indirect_buffer_state = Get<vvl::Buffer>(buffer);
+    if (!indirect_buffer_state) {
+        InternalError(LogObjectList(commandBuffer, buffer), record_obj.location, "buffer must be a valid VkBuffer handle");
+        return;
+    }
+    InsertIndirectMeshDrawValidation(*this, record_obj.location, *cb_state, *indirect_buffer_state, offset, maxDrawCount,
+                                     countBuffer, countBufferOffset, stride);
     SetupShaderInstrumentationResources(*this, *cb_state, VK_PIPELINE_BIND_POINT_GRAPHICS, record_obj.location);
 }
 

diff --git a/layers/gpu/error_message/gpuav_vuids.cpp b/layers/gpu/error_message/gpuav_vuids.cpp
@@ -46,6 +46,9 @@ struct GpuVuidsCmdDrawIndexed : GpuVuid {
         uniform_access_oob_08612 = "VUID-vkCmdDrawIndexed-None-08612";
         storage_access_oob_08613 = "VUID-vkCmdDrawIndexed-None-08613";
         invalid_descriptor = "VUID-vkCmdDrawIndexed-None-08114";
+        // This should be detected on the host by core validation but just in case...
+        index_buffer_size = "VUID-vkCmdDrawIndexedEXT-robustBufferAccess2-08798";
+        vertex_index_oob = "VUID-vkCmdDrawIndexed-None-02721";
     }
 };
 
@@ -56,6 +59,8 @@ struct GpuVuidsCmdDrawMultiIndexedEXT : GpuVuid {
         uniform_access_oob_08612 = "VUID-vkCmdDrawMultiIndexedEXT-None-08612";
         storage_access_oob_08613 = "VUID-vkCmdDrawMultiIndexedEXT-None-08613";
         invalid_descriptor = "VUID-vkCmdDrawMultiIndexedEXT-None-08114";
+        index_buffer_size = "VUID-vkCmdDrawMultiIndexedEXT-robustBufferAccess2-08798";
+        vertex_index_oob = "VUID-vkCmdDrawMultiIndexedEXT-None-02721";
     }
 };
 
@@ -78,6 +83,8 @@ struct GpuVuidsCmdDrawIndexedIndirect : GpuVuid {
         storage_access_oob_08613 = "VUID-vkCmdDrawIndexedIndirect-None-08613";
         invalid_descriptor = "VUID-vkCmdDrawIndexedIndirect-None-08114";
         first_instance_not_zero = "VUID-VkDrawIndexedIndirectCommand-firstInstance-00554";
+        index_buffer_size = "VUID-vkCmdDrawIndexedIndirect-robustBufferAccess2-08798";
+        vertex_index_oob = "VUID-vkCmdDrawIndexedIndirect-None-02721";
     }
 };
 
@@ -128,6 +135,8 @@ struct GpuVuidsCmdDrawIndexedIndirectCount : GpuVuid {
         count_exceeds_bufsize_1 = "VUID-vkCmdDrawIndexedIndirectCount-countBuffer-03153";
         count_exceeds_bufsize = "VUID-vkCmdDrawIndexedIndirectCount-countBuffer-03154";
         count_exceeds_device_limit = "VUID-vkCmdDrawIndexedIndirectCount-countBuffer-02717";
+        index_buffer_size = "VUID-vkCmdDrawIndexedIndirectCount-robustBufferAccess2-08798";
+        vertex_index_oob = "VUID-vkCmdDrawIndexedIndirectCount-None-02721";
     }
 };
 

diff --git a/layers/gpu/error_message/gpuav_vuids.h b/layers/gpu/error_message/gpuav_vuids.h
@@ -46,6 +46,8 @@ struct GpuVuid {
     const char* trace_rays_width_exceeds_device_limit = kVUIDUndefined;
     const char* trace_rays_height_exceeds_device_limit = kVUIDUndefined;
     const char* trace_rays_depth_exceeds_device_limit = kVUIDUndefined;
+    const char* index_buffer_size = kVUIDUndefined;
+    const char* vertex_index_oob = kVUIDUndefined;
 };
 
 // Getter function to provide kVUIDUndefined in case an invalid function is passed in

diff --git a/layers/gpu/resources/gpu_resources.cpp b/layers/gpu/resources/gpu_resources.cpp
@@ -135,7 +135,7 @@ void DescriptorSetManager::PutBackDescriptorSet(VkDescriptorPool desc_pool, VkDe
     return;
 }
 
-void SharedResourcesManager::Clear() {
+void SharedResourcesCache::Clear() {
     for (auto &[key, value] : shared_validation_resources_map_) {
         auto &[object, destructor] = value;
         destructor(object);

diff --git a/layers/gpu/resources/gpu_resources.h b/layers/gpu/resources/gpu_resources.h
@@ -72,7 +72,8 @@ class GpuResourcesManager {
     std::vector<gpu::DeviceMemoryBlock> mem_blocks_;
 };
 
-class SharedResourcesManager {
+// Cache a single object of type T. Key is *only* based on typeid(T)
+class SharedResourcesCache {
   public:
     template <typename T>
     T *TryGet() {
@@ -84,6 +85,10 @@ class SharedResourcesManager {
         return t;
     }
 
+    // First call to Get<T> will create the object, subsequent calls will retrieve the cached entry.
+    // /!\ The cache key is only based on the type T, not on the passed parameters
+    // => Successive calls to Get<T> with different parameters will NOT give different objects,
+    // only the entry cached upon the first call to Get<T> will be retrieved
     template <typename T, class... ConstructorTypes>
     T &Get(ConstructorTypes &&...args) {
         T *t = TryGet<T>();