From a93b1275cc322b11e086d008116b259ac1ecdff9 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: (cherry picked from commit bef37336fb08aaee0bd54d359a9e7fbecbf88b86) --- .pick_status.json | 2 +- src/vulkan/runtime/vk_queue.c | 68 +++++++++++++------------ src/vulkan/runtime/vk_synchronization.c | 22 +++----- 3 files changed, 45 insertions(+), 47 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index 20cb530f1a2..871814a2ec9 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -4724,7 +4724,7 @@ "description": "vulkan/queue: Fix VkTimelineSemaphoreSubmitInfo sanitization", "nominated": false, "nomination_type": 0, - "resolution": 4, + "resolution": 1, "main_sha": null, "because_sha": null, "notes": null diff --git a/src/vulkan/runtime/vk_queue.c b/src/vulkan/runtime/vk_queue.c index 89a046a4008..7f473ae1f2f 100644 --- a/src/vulkan/runtime/vk_queue.c +++ b/src/vulkan/runtime/vk_queue.c @@ -1457,36 +1457,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); @@ -1500,18 +1470,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 6fd562aef59..4886a80c8e5 100644 --- a/src/vulkan/runtime/vk_synchronization.c +++ b/src/vulkan/runtime/vk_synchronization.c @@ -397,14 +397,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); @@ -412,8 +404,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 @@ -424,12 +416,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, }; @@ -444,8 +437,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 @@ -456,12 +449,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, };