Skip to content

Commit

Permalink
Fix buffer overflow when the count of display modes exceeds MaxModePe…
Browse files Browse the repository at this point in the history 
…rScreen(64) (#180)

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

Co-authored-by: Li, Wenqing <[email protected]>
  • Loading branch information
@qiaojbao @WenqingLiAMD
qiaojbao and WenqingLiAMD authored Nov 5, 2024
1 parent 8e7968c commit 786ec85
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 72 deletions.
2 changes: 1 addition & 1 deletion icd/api/include/vk_instance.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ class Instance
{
Pal::IScreen* pPalScreen;
uint32_t modeCount;
Pal::ScreenMode* pModeList[Pal::MaxModePerScreen];
Pal::ScreenMode* pModeList;
};

uint32_t m_screenCount;
Expand Down
55 changes: 18 additions & 37 deletions icd/api/vk_instance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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<Pal::ScreenMode*>(
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<Pal::ScreenMode*>(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<Pal::ScreenMode*>(
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;
}
}

Expand Down
74 changes: 40 additions & 34 deletions icd/api/vk_physical_device.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9754,38 +9754,45 @@ VkResult PhysicalDevice::GetDisplayModeProperties(
Pal::IScreen* pScreen = reinterpret_cast<Pal::IScreen*>(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<DisplayModeObject*>(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<VkDisplayModeKHR>(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<DisplayModeObject*>(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<VkDisplayModeKHR>(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;
}

Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 786ec85

Please sign in to comment.