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)

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
  • Loading branch information
@WenqingLiAMD @qiaojbao
WenqingLiAMD authored and qiaojbao committed Nov 5, 2024
1 parent 8e7968c commit b582244
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 b582244

Please sign in to comment.