From bef37336fb08aaee0bd54d359a9e7fbecbf88b86 Mon Sep 17 00:00:00 2001 From: Connor Abbott Date: Mon, 25 Aug 2025 11:32:10 -0400 Subject: [PATCH] vulkan/queue: Fix VkTimelineSemaphoreSubmitInfo sanitization We're supposed to completely ignore VkTimelineSemaphoreSubmitInfo if there aren't any timeline semaphores, including the array lengths, which is made clear by the various VUs already cited by the code. The vkQueueSubmit() path correctly handled this when asserting but still dereferenced pWaitSemaphoreValues unconditionally, which could lead to dereferencing an invalid pointer if waitSemaphoreValueCount is less than waitSemaphoreCount. The vkQueueSparseBind() path didn't even assert correctly. Bring vkQueueSparseBind() in line with vkQueueSubmit() and make both only dereference the wait/signal array once we've determined it must be present. While we're here, also fix the assert in vkQueueSubmit() to disallow a waitSemaphoreValueCount of 0 if there are timeline semaphores present, which conversely is not allowed. Part-of: --- src/vulkan/runtime/vk_queue.c | 68 +++++++++++++------------ src/vulkan/runtime/vk_synchronization.c | 22 +++----- 2 files changed, 44 insertions(+), 46 deletions(-) diff --git a/src/vulkan/runtime/vk_queue.c b/src/vulkan/runtime/vk_queue.c index 609b8e63096..3d08a6a865d 100644 --- a/src/vulkan/runtime/vk_queue.c +++ b/src/vulkan/runtime/vk_queue.c @@ -1271,36 +1271,6 @@ vk_common_QueueBindSparse(VkQueue _queue, for (uint32_t i = 0; i < bindInfoCount; i++) { const VkTimelineSemaphoreSubmitInfo *timeline_info = vk_find_struct_const(pBindInfo[i].pNext, TIMELINE_SEMAPHORE_SUBMIT_INFO); - const uint64_t *wait_values = NULL; - const uint64_t *signal_values = NULL; - - if (timeline_info && timeline_info->waitSemaphoreValueCount) { - /* From the Vulkan 1.3.204 spec: - * - * VUID-VkBindSparseInfo-pNext-03248 - * - * "If the pNext chain of this structure includes a VkTimelineSemaphoreSubmitInfo structure - * and any element of pSignalSemaphores was created with a VkSemaphoreType of - * VK_SEMAPHORE_TYPE_TIMELINE, then its signalSemaphoreValueCount member must equal - * signalSemaphoreCount" - */ - assert(timeline_info->waitSemaphoreValueCount == pBindInfo[i].waitSemaphoreCount); - wait_values = timeline_info->pWaitSemaphoreValues; - } - - if (timeline_info && timeline_info->signalSemaphoreValueCount) { - /* From the Vulkan 1.3.204 spec: - * - * VUID-VkBindSparseInfo-pNext-03247 - * - * "If the pNext chain of this structure includes a VkTimelineSemaphoreSubmitInfo structure - * and any element of pWaitSemaphores was created with a VkSemaphoreType of - * VK_SEMAPHORE_TYPE_TIMELINE, then its waitSemaphoreValueCount member must equal - * waitSemaphoreCount" - */ - assert(timeline_info->signalSemaphoreValueCount == pBindInfo[i].signalSemaphoreCount); - signal_values = timeline_info->pSignalSemaphoreValues; - } STACK_ARRAY(VkSemaphoreSubmitInfo, wait_semaphore_infos, pBindInfo[i].waitSemaphoreCount); @@ -1314,18 +1284,52 @@ vk_common_QueueBindSparse(VkQueue _queue, } for (uint32_t j = 0; j < pBindInfo[i].waitSemaphoreCount; j++) { + VK_FROM_HANDLE(vk_semaphore, semaphore, pBindInfo[i].pWaitSemaphores[j]); + + uint64_t wait_value = 0; + if (timeline_info && semaphore->type == VK_SEMAPHORE_TYPE_TIMELINE) { + /* From the Vulkan 1.3.204 spec: + * + * VUID-VkBindSparseInfo-pNext-03248 + * + * "If the pNext chain of this structure includes a VkTimelineSemaphoreSubmitInfo structure + * and any element of pSignalSemaphores was created with a VkSemaphoreType of + * VK_SEMAPHORE_TYPE_TIMELINE, then its signalSemaphoreValueCount member must equal + * signalSemaphoreCount" + */ + assert(timeline_info->waitSemaphoreValueCount == pBindInfo[i].waitSemaphoreCount); + wait_value = timeline_info->pWaitSemaphoreValues[j]; + } + wait_semaphore_infos[j] = (VkSemaphoreSubmitInfo) { .sType = VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO, .semaphore = pBindInfo[i].pWaitSemaphores[j], - .value = wait_values ? wait_values[j] : 0, + .value = wait_value, }; } for (uint32_t j = 0; j < pBindInfo[i].signalSemaphoreCount; j++) { + VK_FROM_HANDLE(vk_semaphore, semaphore, pBindInfo[i].pSignalSemaphores[j]); + + uint64_t signal_value = 0; + if (timeline_info && semaphore->type == VK_SEMAPHORE_TYPE_TIMELINE) { + /* From the Vulkan 1.3.204 spec: + * + * VUID-VkBindSparseInfo-pNext-03247 + * + * "If the pNext chain of this structure includes a VkTimelineSemaphoreSubmitInfo structure + * and any element of pWaitSemaphores was created with a VkSemaphoreType of + * VK_SEMAPHORE_TYPE_TIMELINE, then its waitSemaphoreValueCount member must equal + * waitSemaphoreCount" + */ + assert(timeline_info->signalSemaphoreValueCount == pBindInfo[i].signalSemaphoreCount); + signal_value = timeline_info->pSignalSemaphoreValues[j]; + } + signal_semaphore_infos[j] = (VkSemaphoreSubmitInfo) { .sType = VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO, .semaphore = pBindInfo[i].pSignalSemaphores[j], - .value = signal_values ? signal_values[j] : 0, + .value = signal_value, }; } struct vulkan_submit_info info = { diff --git a/src/vulkan/runtime/vk_synchronization.c b/src/vulkan/runtime/vk_synchronization.c index 7ee6ef1cfc6..092d9577efe 100644 --- a/src/vulkan/runtime/vk_synchronization.c +++ b/src/vulkan/runtime/vk_synchronization.c @@ -396,14 +396,6 @@ vk_common_QueueSubmit( const VkTimelineSemaphoreSubmitInfo *timeline_info = vk_find_struct_const(pSubmits[s].pNext, TIMELINE_SEMAPHORE_SUBMIT_INFO); - const uint64_t *wait_values = NULL; - const uint64_t *signal_values = NULL; - - if (timeline_info && timeline_info->waitSemaphoreValueCount) - wait_values = timeline_info->pWaitSemaphoreValues; - - if (timeline_info && timeline_info->signalSemaphoreValueCount) - signal_values = timeline_info->pSignalSemaphoreValues; const VkDeviceGroupSubmitInfo *group_info = vk_find_struct_const(pSubmits[s].pNext, DEVICE_GROUP_SUBMIT_INFO); @@ -411,8 +403,8 @@ vk_common_QueueSubmit( for (uint32_t i = 0; i < pSubmits[s].waitSemaphoreCount; i++) { VK_FROM_HANDLE(vk_semaphore, semaphore, pSubmits[s].pWaitSemaphores[i]); - if (semaphore->type == VK_SEMAPHORE_TYPE_TIMELINE && - timeline_info && timeline_info->waitSemaphoreValueCount) { + uint64_t wait_value = 0; + if (semaphore->type == VK_SEMAPHORE_TYPE_TIMELINE && timeline_info) { /* From the Vulkan 1.3.204 spec: * * VUID-VkSubmitInfo-pNext-03241 @@ -423,12 +415,13 @@ vk_common_QueueSubmit( * waitSemaphoreCount" */ assert(timeline_info->waitSemaphoreValueCount == pSubmits[s].waitSemaphoreCount); + wait_value = timeline_info->pWaitSemaphoreValues[i]; } wait_semaphores[n_wait_semaphores + i] = (VkSemaphoreSubmitInfo) { .sType = VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO, .semaphore = pSubmits[s].pWaitSemaphores[i], - .value = wait_values ? wait_values[i] : 0, + .value = wait_value, .stageMask = pSubmits[s].pWaitDstStageMask[i], .deviceIndex = group_info ? group_info->pWaitSemaphoreDeviceIndices[i] : 0, }; @@ -443,8 +436,8 @@ vk_common_QueueSubmit( for (uint32_t i = 0; i < pSubmits[s].signalSemaphoreCount; i++) { VK_FROM_HANDLE(vk_semaphore, semaphore, pSubmits[s].pSignalSemaphores[i]); - if (semaphore->type == VK_SEMAPHORE_TYPE_TIMELINE && - timeline_info && timeline_info->signalSemaphoreValueCount) { + uint64_t signal_value = 0; + if (semaphore->type == VK_SEMAPHORE_TYPE_TIMELINE && timeline_info) { /* From the Vulkan 1.3.204 spec: * * VUID-VkSubmitInfo-pNext-03240 @@ -455,12 +448,13 @@ vk_common_QueueSubmit( * signalSemaphoreCount" */ assert(timeline_info->signalSemaphoreValueCount == pSubmits[s].signalSemaphoreCount); + signal_value = timeline_info->pSignalSemaphoreValues[i]; } signal_semaphores[n_signal_semaphores + i] = (VkSemaphoreSubmitInfo) { .sType = VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO, .semaphore = pSubmits[s].pSignalSemaphores[i], - .value = signal_values ? signal_values[i] : 0, + .value = signal_value, .stageMask = VK_PIPELINE_STAGE_2_ALL_COMMANDS_BIT, .deviceIndex = group_info ? group_info->pSignalSemaphoreDeviceIndices[i] : 0, };