From 1ab23b35c3a1f38fb198980b05f58303c4b47031 Mon Sep 17 00:00:00 2001 From: metsw24-max Date: Thu, 11 Jun 2026 17:00:14 +0530 Subject: [PATCH] Fix off-by-one heap buffer overflow in Windows registry manifest path accumulation --- loader/loader_windows.c | 11 ++++++--- tests/framework/shim/windows_shim.cpp | 7 ++++-- tests/loader_regression_tests.cpp | 33 +++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/loader/loader_windows.c b/loader/loader_windows.c index a02582ccd..81d2fedca 100644 --- a/loader/loader_windows.c +++ b/loader/loader_windows.c @@ -462,7 +462,8 @@ VkResult windows_get_registry_files(const struct loader_instance *inst, char *lo goto out; } *reg_data[0] = '\0'; - } else if (strlen(*reg_data) + name_size + 1 > *reg_data_size) { + // Reserve room for the existing contents, a PATH_SEPARATOR, the new value, and the null terminator. + } else if (strlen(*reg_data) + name_size + 2 > *reg_data_size) { void *new_ptr = loader_instance_heap_realloc(inst, *reg_data, *reg_data_size, *reg_data_size * 2, VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE); if (NULL == new_ptr) { @@ -534,7 +535,8 @@ VkResult windows_get_registry_files(const struct loader_instance *inst, char *lo if (strlen(*reg_data) == 0) { // The list is emtpy. Add the first entry. - (void)snprintf(*reg_data, name_size + 1, "%s", name); + // Bound the write by the actual destination size, not the source length. + (void)snprintf(*reg_data, *reg_data_size, "%s", name); found = true; } else { // At this point the reg_data variable contains other JSON paths, likely from the PNP/device section @@ -554,7 +556,10 @@ VkResult windows_get_registry_files(const struct loader_instance *inst, char *lo // Only skip if we are adding a driver and a duplicate was found if (!is_driver || (is_driver && foundDuplicate == false)) { // Add the new entry to the list. - (void)snprintf(*reg_data + strlen(*reg_data), name_size + 2, "%c%s", PATH_SEPARATOR, name); + // Bound the write by the remaining destination size, not the source length, so the + // PATH_SEPARATOR + name + null terminator can never spill past the allocation. + size_t cur_len = strlen(*reg_data); + (void)snprintf(*reg_data + cur_len, *reg_data_size - cur_len, "%c%s", PATH_SEPARATOR, name); found = true; } else { loader_log( diff --git a/tests/framework/shim/windows_shim.cpp b/tests/framework/shim/windows_shim.cpp index 1c6bc0b9f..1437f2fc7 100644 --- a/tests/framework/shim/windows_shim.cpp +++ b/tests/framework/shim/windows_shim.cpp @@ -337,12 +337,15 @@ LSTATUS __stdcall ShimRegEnumValueA(HKEY hKey, DWORD dwIndex, LPSTR lpValueName, if (dwIndex >= location.size()) return ERROR_NO_MORE_ITEMS; std::string name = narrow(location[dwIndex].name); - if (*lpcchValueName < name.size()) return ERROR_NO_MORE_ITEMS; + // The destination buffer must hold the name plus a null terminator, matching the real RegEnumValueA contract. + if (*lpcchValueName <= name.size()) return ERROR_NO_MORE_ITEMS; for (size_t i = 0; i < name.size(); i++) { lpValueName[i] = name[i]; } lpValueName[name.size()] = '\0'; - *lpcchValueName = static_cast(name.size() + 1); + // RegEnumValueA reports the number of characters in the name *excluding* the null terminator. Reporting the + // length including the terminator (off by one) hides destination-buffer-sizing bugs in callers, so match the API. + *lpcchValueName = static_cast(name.size()); if (*lpcbData < sizeof(DWORD)) return ERROR_NO_MORE_ITEMS; DWORD *lpcbData_dword = reinterpret_cast(lpData); *lpcbData_dword = location[dwIndex].value; diff --git a/tests/loader_regression_tests.cpp b/tests/loader_regression_tests.cpp index 8b90fef31..789c1b6cb 100644 --- a/tests/loader_regression_tests.cpp +++ b/tests/loader_regression_tests.cpp @@ -4361,6 +4361,39 @@ TEST(RegistryManifestParsing, OversizedD3DKMTDriverPathDoesNotOverflow) { InstWrapper inst{env.vulkan_functions}; inst.CheckCreate(VK_ERROR_INCOMPATIBLE_DRIVER); } + +// Regression test for an off-by-one heap buffer overflow in windows_get_registry_files(). That function accumulates +// manifest paths enumerated from the generic Khronos\Vulkan registry locations into a heap buffer that starts at 4096 +// bytes and grows by doubling. Each appended entry is written as PATH_SEPARATOR + name + null terminator, but the +// grow check only reserved name + null (it forgot the separator). When the running length lined up so that +// strlen(reg_data) + name_size + 1 == reg_data_size exactly, the buffer was not grown and the terminating null byte +// was written one element past the end of the allocation. +// +// The registry value names below are sized so the third append lands the write exactly on the 4096-byte boundary: +// entry 0: strlen -> 2000 +// entry 1: +sep+name strlen -> 2000 + 1 + 2000 = 4001 +// entry 2: +sep+name writes 1 + 94 + 1 = 96 bytes at offset 4001 -> last index 4096 (one past a 4096-byte buffer) +// 2000 + 2000 + 94 == 4094, and 4001 + 94 + 1 == 4096, so the unpatched grow check (which compares against +// name_size + 1) does not trigger a realloc and the write overflows by a single null byte. Run under ASAN this +// aborts before the fix; with the fix the buffer is grown to fit and instance creation completes cleanly. +TEST(RegistryManifestParsing, AppendAtBufferBoundaryDoesNotOverflow) { + FrameworkEnvironment env{}; + env.add_icd(TEST_ICD_PATH_VERSION_2).add_physical_device("physical_device_0"); + + auto make_named_path = [](size_t total_len) { + const std::string suffix = ".json"; + return std::filesystem::path(std::string(total_len - suffix.size(), 'a') + suffix); + }; + + env.platform_shim->add_manifest_to_registry(ManifestCategory::explicit_layer, make_named_path(2000)); + env.platform_shim->add_manifest_to_registry(ManifestCategory::explicit_layer, make_named_path(2000)); + env.platform_shim->add_manifest_to_registry(ManifestCategory::explicit_layer, make_named_path(94)); + + // None of the registry entries point at a real manifest, so no layers are added - the loader simply needs to + // enumerate the (bogus) paths without overflowing its accumulation buffer. + InstWrapper inst{env.vulkan_functions}; + inst.CheckCreate(); +} #endif TEST(LibraryLoading, SystemLocations) {