AMDVLK out of bounds write causes memory corruption/crash when an attached display monitor has more than 64 video modes.

[kleinerm]

Short summary:

If a Vulkan client on Linux tries to call vkGetDisplayModePropertiesKHR() on a video output which exposes more than 64 video modes then amdvlk experiences memory and stack corruption due to out-of-bounds writes, leading to client crash/termination, e.g., "Stack smashing detected" abort().

Explanation of how/why this happens:

If a Vulkan client on Linux tries to call vkGetDisplayModePropertiesKHR() on a VKDisplayKHR (running on a virtual terminal, or created from a RandR leased output or similar), then the following call-chain is executed in XGL's vk_instance.cpp:

vk_physical_device.cpp: vkGetDisplayModePropertiesKHR() -> vk_physical_device.cpp: PhysicalDevice::GetDisplayModeProperties()-> vk_instance.cpp: Instance::GetScreenModeList() -> Multiple calls to pScreen->GetScreenModeList() implemented in PAL, which will then call into the Linux OS specific amdgpuDevice.cpp : Device::QueryScreenModesForConnector()

Device::QueryScreenModesForConnector() returns the number of video modes supported on the Linux DRM connector for the video output associated with the VKDisplayKHR handle, or a list of all the video modes. It returns the full count, or the full list of modes.

This is a problem, because the current implementation of XGL uses multiple arrays for storing or caching these returned video modes, which have a fixed maximum size of Pal::MaxModePerScreen = 64 elements, as defined in palPlatform.h.

For example pModeList in struct vk_instance.h ScreenObject

xgl/icd/api/include/vk_instance.h

Line 355 in c4ca1f6

Pal::ScreenMode* pModeList[Pal::MaxModePerScreen];

and in

xgl/icd/api/vk_physical_device.cpp

Line 9534 in c4ca1f6

Pal::ScreenMode* pScreenMode[Pal::MaxModePerScreen];

are arrays which are involved in the above call sequence and have a fixed maximum capacity of 64 video modes.

Therefore calling vkGetDisplayModePropertiesKHR() on a display with > 64 modes will cause out-of-bounds writes into these arrays, corrupting data structures inside the VKInstance and also stack corruption, leasing to malfunction or crash of the client app.

Possible solutions?

Ideally there wouldn't be fixed size arrays like those mentioned above. Or at least the limit of Pal::MaxModePerScreen inside
https://github.com/GPUOpen-Drivers/pal/blob/31f6a70d0e9ee22894daab5305dd91b3ebb599a5/inc/core/palPlatform.h#L88
should be raised to a much larger number.

Then the code inside Instance::GetScreenModeList() should do checking on the modecount values returned by Pal and clamp the returned mode counts and internally cached modes to a maximum of Pal::MaxModePerScreen to prevent future out-of-bounds writes due to array overflows.

xgl/icd/api/vk_instance.cpp

Line 868 in c4ca1f6

VkResult Instance::GetScreenModeList(

Thanks.

[jinjianrong]

The fix will be in next release

[kleinerm]

Great! Did you bump the Pal::MaxModePerScreen to a higher value and implement suitable bounds checking/clamping to prevent a future repetition of the bug? Or even switch to dynamic allocation?

For reference, a user of my software which first reported this crash, experienced it with a HDR projector that exposed 107 video modes. So if you just bumped the static limit, but didn't switch to dynamic allocation, I would propose choosing at least a limit of 256 modes at a minimum, or maybe even a more generous limit of 512 modes to be on the relative safe side.

Thanks!

[jinjianrong]

Here is the fix : #180

[kleinerm]

I reviewed the code of your fix and it looks good to me. Dynamic allocation, great!
I can't test this myself easily anymore, as the current driver no longer supports the Polaris and Vega gpu's I have, sadly only RDNA. But I will let my user who has a RDNA gpu know about the fix. Maybe he can test it already and then I can close this issue.
Thanks.