From b5822444763750ae91831a20507d6f3155e21dfc Mon Sep 17 00:00:00 2001 From: "Li, Wenqing" Date: Tue, 15 Oct 2024 13:27:20 +0000 Subject: [PATCH] Fix buffer overflow when the count of display modes exceeds MaxModePerScreen(64) 1. Free pModeList memory when destroying instance 2. Add protection when allocating memory for pModeList fails 3. Avoid using an early return 4. Remove the if condition of ppModeList == nullptr, it doesn't match the new calling conventions 5. Always initialize variables --- icd/api/include/vk_instance.h | 2 +- icd/api/vk_instance.cpp | 55 +++++++++---------------- icd/api/vk_physical_device.cpp | 74 ++++++++++++++++++---------------- 3 files changed, 59 insertions(+), 72 deletions(-) diff --git a/icd/api/include/vk_instance.h b/icd/api/include/vk_instance.h index f0771fa5..9237eff4 100644 --- a/icd/api/include/vk_instance.h +++ b/icd/api/include/vk_instance.h @@ -352,7 +352,7 @@ class Instance { Pal::IScreen* pPalScreen; uint32_t modeCount; - Pal::ScreenMode* pModeList[Pal::MaxModePerScreen]; + Pal::ScreenMode* pModeList; }; uint32_t m_screenCount; diff --git a/icd/api/vk_instance.cpp b/icd/api/vk_instance.cpp index af3e7ad3..d963de9d 100644 --- a/icd/api/vk_instance.cpp +++ b/icd/api/vk_instance.cpp @@ -697,6 +697,7 @@ VkResult Instance::Destroy(void) for (uint32_t i = 0; i < m_screenCount; ++i) { m_screens[i].pPalScreen->Destroy(); + FreeMem(m_screens[i].pModeList); } FreeMem(m_pScreenStorage); @@ -865,6 +866,8 @@ VkResult Instance::EnumerateExtensionProperties( // ===================================================================================================================== // Get mode list for specific screen. +// NOTE: this function modifies the *ppModeList to make it point to the internal ScreenMode array of instance, instead +// filling in the input array. VkResult Instance::GetScreenModeList( const Pal::IScreen* pScreen, uint32_t* pModeCount, @@ -877,51 +880,29 @@ VkResult Instance::GetScreenModeList( { if (m_screens[screenIdx].pPalScreen == pScreen) { - if (ppModeList == nullptr) + if (m_screens[screenIdx].pModeList == nullptr) { - palResult = pScreen->GetScreenModeList(pModeCount, nullptr); + uint32_t modeCount = 0; + palResult = pScreen->GetScreenModeList(&modeCount, nullptr); VK_ASSERT(palResult == Pal::Result::Success); - } - else - { - if (m_screens[screenIdx].pModeList[0] == nullptr) - { - uint32_t modeCount = 0; - palResult = pScreen->GetScreenModeList(&modeCount, nullptr); - VK_ASSERT(palResult == Pal::Result::Success); - - m_screens[screenIdx].pModeList[0] = reinterpret_cast( - AllocMem(modeCount * sizeof(Pal::ScreenMode), VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE)); - - for (uint32_t i = 1; i < modeCount; ++i) - { - m_screens[screenIdx].pModeList[i] = reinterpret_cast(Util::VoidPtrInc( - m_screens[screenIdx].pModeList[0], - i * sizeof(Pal::ScreenMode))); - } - - palResult = pScreen->GetScreenModeList(&modeCount, m_screens[screenIdx].pModeList[0]); - VK_ASSERT(palResult == Pal::Result::Success); - m_screens[screenIdx].modeCount = modeCount; - } - - uint32_t loopCount = m_screens[screenIdx].modeCount; - - if (*pModeCount < m_screens[screenIdx].modeCount) + m_screens[screenIdx].pModeList = reinterpret_cast( + AllocMem(modeCount * sizeof(Pal::ScreenMode), VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE)); + if (m_screens[screenIdx].pModeList == nullptr) { - result = VK_INCOMPLETE; - loopCount = *pModeCount; + result = VK_ERROR_OUT_OF_HOST_MEMORY; + break; } - for (uint32_t i = 0; i < loopCount; i++) - { - ppModeList[i] = m_screens[screenIdx].pModeList[i]; - } + palResult = pScreen->GetScreenModeList(&modeCount, m_screens[screenIdx].pModeList); + VK_ASSERT(palResult == Pal::Result::Success); - *pModeCount = loopCount; - break; + m_screens[screenIdx].modeCount = modeCount; } + + *pModeCount = m_screens[screenIdx].modeCount; + *ppModeList = m_screens[screenIdx].pModeList; + break; } } diff --git a/icd/api/vk_physical_device.cpp b/icd/api/vk_physical_device.cpp index 14629e8c..5ec83b79 100644 --- a/icd/api/vk_physical_device.cpp +++ b/icd/api/vk_physical_device.cpp @@ -9754,38 +9754,45 @@ VkResult PhysicalDevice::GetDisplayModeProperties( Pal::IScreen* pScreen = reinterpret_cast(display); VK_ASSERT(pScreen != nullptr); - if (properties.IsNull()) + Pal::ScreenMode* pScreenMode = nullptr; + uint32_t propertyCount = 0; + result = VkInstance()->GetScreenModeList(pScreen, &propertyCount, &pScreenMode); + if (result == VK_SUCCESS) { - return VkInstance()->GetScreenModeList(pScreen, pPropertyCount, nullptr); - } - - Pal::ScreenMode* pScreenMode[Pal::MaxModePerScreen]; - - uint32_t propertyCount = *pPropertyCount; + if (properties.IsNull()) + { + *pPropertyCount = propertyCount; + } + else + { + if (*pPropertyCount < propertyCount) + { + result = VK_INCOMPLETE; + } - result = VkInstance()->GetScreenModeList(pScreen, &propertyCount, pScreenMode); + uint32_t loopCount = Util::Min(*pPropertyCount, propertyCount); - uint32_t loopCount = Util::Min(*pPropertyCount, propertyCount); + for (uint32_t i = 0; i < loopCount; i++) + { + DisplayModeObject* pDisplayMode = + reinterpret_cast(VkInstance()->AllocMem(sizeof(DisplayModeObject), + VK_DEFAULT_MEM_ALIGN, + VK_SYSTEM_ALLOCATION_SCOPE_OBJECT)); + pDisplayMode->pScreen = pScreen; + memcpy(&pDisplayMode->palScreenMode, &pScreenMode[i], sizeof(Pal::ScreenMode)); + properties[i].displayMode = reinterpret_cast(pDisplayMode); + properties[i].parameters.visibleRegion.width = pScreenMode[i].extent.width; + properties[i].parameters.visibleRegion.height = pScreenMode[i].extent.height; + // Spec requires refresh rate to be "the number of times the display is refreshed each second + // multiplied by 1000", in other words, HZ * 1000 + properties[i].parameters.refreshRate = + pScreenMode[i].refreshRate.numerator * 1000 / pScreenMode[i].refreshRate.denominator; + } - for (uint32_t i = 0; i < loopCount; i++) - { - DisplayModeObject* pDisplayMode = - reinterpret_cast(VkInstance()->AllocMem(sizeof(DisplayModeObject), - VK_DEFAULT_MEM_ALIGN, - VK_SYSTEM_ALLOCATION_SCOPE_OBJECT)); - pDisplayMode->pScreen = pScreen; - memcpy(&pDisplayMode->palScreenMode, pScreenMode[i], sizeof(Pal::ScreenMode)); - properties[i].displayMode = reinterpret_cast(pDisplayMode); - properties[i].parameters.visibleRegion.width = pScreenMode[i]->extent.width; - properties[i].parameters.visibleRegion.height = pScreenMode[i]->extent.height; - // Spec requires refresh rate to be "the number of times the display is refreshed each second - // multiplied by 1000", in other words, HZ * 1000 - properties[i].parameters.refreshRate = - pScreenMode[i]->refreshRate.numerator * 1000 / pScreenMode[i]->refreshRate.denominator; + *pPropertyCount = loopCount; + } } - *pPropertyCount = loopCount; - return result; } @@ -9833,20 +9840,19 @@ VkResult PhysicalDevice::CreateDisplayMode( VkResult result = VK_SUCCESS; - Pal::ScreenMode* pScreenMode[Pal::MaxModePerScreen]; - uint32_t propertyCount = Pal::MaxModePerScreen; - - VkInstance()->GetScreenModeList(pScreen, &propertyCount, pScreenMode); - + Pal::ScreenMode* pScreenMode = nullptr; + uint32_t propertyCount = 0; + result = VkInstance()->GetScreenModeList(pScreen, &propertyCount, &pScreenMode); + VK_ASSERT(result == VK_SUCCESS); bool isValidMode = false; for (uint32_t i = 0; i < propertyCount; i++) { // The modes are considered as identical if the dimension as well as the refresh rate are the same. - if ((pCreateInfo->parameters.visibleRegion.width == pScreenMode[i]->extent.width) && - (pCreateInfo->parameters.visibleRegion.height == pScreenMode[i]->extent.height) && + if ((pCreateInfo->parameters.visibleRegion.width == pScreenMode[i].extent.width) && + (pCreateInfo->parameters.visibleRegion.height == pScreenMode[i].extent.height) && (pCreateInfo->parameters.refreshRate == - pScreenMode[i]->refreshRate.numerator * 1000 / pScreenMode[i]->refreshRate.denominator)) + pScreenMode[i].refreshRate.numerator * 1000 / pScreenMode[i].refreshRate.denominator)) { isValidMode = true; break;