From 1e09b25e96fbcb8ab7b40a07ef21afb89ffe54f7 Mon Sep 17 00:00:00 2001 From: Charles Giessen Date: Tue, 22 Aug 2023 15:49:17 -0600 Subject: Add device dispatch table magic value check The device dispatch table has a magic value to ensure consistency, but there was no message logged in case the magic value was corrupted. This commit adds that check, fixes the message being printed for the instance magic value check, and adds tests to ensure they are contining to be checked in the future. --- docs/LoaderLayerInterface.md | 16 +++++++ loader/generated/vk_loader_extensions.c | 2 +- loader/loader.c | 13 +++++- loader/trampoline.c | 32 +++++--------- scripts/loader_extension_generator.py | 2 +- tests/framework/layer/test_layer.cpp | 8 ++++ tests/framework/layer/test_layer.h | 5 +++ tests/loader_regression_tests.cpp | 77 +++++++++++++++++++++++++++++++++ 8 files changed, 131 insertions(+), 24 deletions(-) diff --git a/docs/LoaderLayerInterface.md b/docs/LoaderLayerInterface.md index a4406955..2ee61441 100644 --- a/docs/LoaderLayerInterface.md +++ b/docs/LoaderLayerInterface.md @@ -2500,6 +2500,22 @@ Android Vulkan documentation. Yes N/A + + LLP_LAYER_22 + During vkCreateDevice, a layer must not modify the + pDevice pointer during prior to calling down to the lower + layers.
+ This is because the loader passes information in this pointer that is + necessary for the initialization code in the loader's terminator + function.
+ Instead, if the layer is overriding the pDevice pointer, it + must do so only after the call to the lower layers returns. + + The loader will likely crash. + No + Yes + N/A + diff --git a/loader/generated/vk_loader_extensions.c b/loader/generated/vk_loader_extensions.c index 97df84de..411f9729 100644 --- a/loader/generated/vk_loader_extensions.c +++ b/loader/generated/vk_loader_extensions.c @@ -316,7 +316,7 @@ VKAPI_ATTR bool VKAPI_CALL loader_icd_init_entries(struct loader_icd_term *icd_t VKAPI_ATTR void VKAPI_CALL loader_init_device_dispatch_table(struct loader_dev_dispatch_table *dev_table, PFN_vkGetDeviceProcAddr gpa, VkDevice dev) { VkLayerDispatchTable *table = &dev_table->core_dispatch; - assert(table->magic == DEVICE_DISP_TABLE_MAGIC_NUMBER); + if (table->magic != DEVICE_DISP_TABLE_MAGIC_NUMBER) { abort(); } for (uint32_t i = 0; i < MAX_NUM_UNKNOWN_EXTS; i++) dev_table->ext_dispatch[i] = (PFN_vkDevExt)vkDevExtError; // ---- Core 1_0 commands diff --git a/loader/loader.c b/loader/loader.c index 52ff44c1..389cbfce 100644 --- a/loader/loader.c +++ b/loader/loader.c @@ -5210,7 +5210,7 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateInstance(const VkInstanceCreateI loader_log(ptr_instance, VULKAN_LOADER_WARN_BIT, 0, "terminator_CreateInstance: Instance pointer (%p) has invalid MAGIC value 0x%08x. Instance value possibly " "corrupted by active layer (Policy #LLP_LAYER_21). ", - ptr_instance->magic); + ptr_instance, ptr_instance->magic); } // Save the application version if it has been modified - layers sometimes needs features in newer API versions than @@ -5593,6 +5593,17 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDevice(VkPhysicalDevice physical VkBaseOutStructure *caller_dgci_container = NULL; VkDeviceGroupDeviceCreateInfoKHR *caller_dgci = NULL; + if (NULL == dev) { + loader_log(icd_term->this_instance, VULKAN_LOADER_WARN_BIT, 0, + "terminator_CreateDevice: Loader device pointer null encountered. Possibly set by active layer. (Policy " + "#LLP_LAYER_22)"); + } else if (LOADER_MAGIC_NUMBER != dev->loader_dispatch.core_dispatch.magic) { + loader_log(icd_term->this_instance, VULKAN_LOADER_WARN_BIT, 0, + "terminator_CreateDevice: Device pointer (%p) has invalid MAGIC value 0x%08x. Device value possibly " + "corrupted by active layer (Policy #LLP_LAYER_22). ", + dev, dev->loader_dispatch.core_dispatch.magic); + } + dev->phys_dev_term = phys_dev_term; icd_exts.list = NULL; diff --git a/loader/trampoline.c b/loader/trampoline.c index fe4acc95..65809efb 100644 --- a/loader/trampoline.c +++ b/loader/trampoline.c @@ -97,10 +97,8 @@ LOADER_EXPORT VKAPI_ATTR PFN_vkVoidFunction VKAPI_CALL vkGetInstanceProcAddr(VkI struct loader_instance *ptr_instance = loader_get_instance(instance); // If we've gotten here and the pointer is NULL, it's invalid if (ptr_instance == NULL) { - loader_log(NULL, - VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | - VULKAN_LOADER_VALIDATION_BIT, - 0, "vkGetInstanceProcAddr: Invalid instance [VUID-vkGetInstanceProcAddr-instance-parameter]"); + loader_log(NULL, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0, + "vkGetInstanceProcAddr: Invalid instance [VUID-vkGetInstanceProcAddr-instance-parameter]"); abort(); /* Intentionally fail so user can correct issue. */ } // Return trampoline code for non-global entrypoints including any extensions. @@ -770,10 +768,8 @@ LOADER_EXPORT VKAPI_ATTR void VKAPI_CALL vkDestroyInstance(VkInstance instance, ptr_instance = loader_get_instance(instance); if (ptr_instance == NULL) { - loader_log( - NULL, - VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, - 0, "vkDestroyInstance: Invalid instance [VUID-vkDestroyInstance-instance-parameter]"); + loader_log(NULL, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0, + "vkDestroyInstance: Invalid instance [VUID-vkDestroyInstance-instance-parameter]"); loader_platform_thread_unlock_mutex(&loader_lock); abort(); /* Intentionally fail so user can correct issue. */ } @@ -829,20 +825,15 @@ LOADER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkEnumeratePhysicalDevices(VkInstan inst = loader_get_instance(instance); if (NULL == inst) { - loader_log( - NULL, - VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, - 0, "vkEnumeratePhysicalDevices: Invalid instance [VUID-vkEnumeratePhysicalDevices-instance-parameter]"); + loader_log(NULL, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0, + "vkEnumeratePhysicalDevices: Invalid instance [VUID-vkEnumeratePhysicalDevices-instance-parameter]"); abort(); /* Intentionally fail so user can correct issue. */ } if (NULL == pPhysicalDeviceCount) { - loader_log( - inst, - VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, - 0, - "vkEnumeratePhysicalDevices: Received NULL pointer for physical device count return value. " - "[VUID-vkEnumeratePhysicalDevices-pPhysicalDeviceCount-parameter]"); + loader_log(inst, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0, + "vkEnumeratePhysicalDevices: Received NULL pointer for physical device count return value. " + "[VUID-vkEnumeratePhysicalDevices-pPhysicalDeviceCount-parameter]"); res = VK_ERROR_INITIALIZATION_FAILED; goto out; } @@ -870,9 +861,8 @@ LOADER_EXPORT VKAPI_ATTR void VKAPI_CALL vkGetPhysicalDeviceFeatures(VkPhysicalD VkPhysicalDevice unwrapped_phys_dev = loader_unwrap_physical_device(physicalDevice); if (VK_NULL_HANDLE == unwrapped_phys_dev) { loader_log( - NULL, - VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, - 0, "vkGetPhysicalDeviceFeatures: Invalid physicalDevice [VUID-vkGetPhysicalDeviceFeatures-physicalDevice-parameter]"); + NULL, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0, + "vkGetPhysicalDeviceFeatures: Invalid physicalDevice [VUID-vkGetPhysicalDeviceFeatures-physicalDevice-parameter]"); abort(); /* Intentionally fail so user can correct issue. */ } disp = loader_get_instance_layer_dispatch(physicalDevice); diff --git a/scripts/loader_extension_generator.py b/scripts/loader_extension_generator.py index c1c70e79..c52ca886 100644 --- a/scripts/loader_extension_generator.py +++ b/scripts/loader_extension_generator.py @@ -780,7 +780,7 @@ class LoaderExtensionOutputGenerator(OutputGenerator): tables += 'VKAPI_ATTR void VKAPI_CALL loader_init_device_dispatch_table(struct loader_dev_dispatch_table *dev_table, PFN_vkGetDeviceProcAddr gpa,\n' tables += ' VkDevice dev) {\n' tables += ' VkLayerDispatchTable *table = &dev_table->core_dispatch;\n' - tables += ' assert(table->magic == DEVICE_DISP_TABLE_MAGIC_NUMBER);\n' + tables += ' if (table->magic != DEVICE_DISP_TABLE_MAGIC_NUMBER) { abort(); }\n' tables += ' for (uint32_t i = 0; i < MAX_NUM_UNKNOWN_EXTS; i++) dev_table->ext_dispatch[i] = (PFN_vkDevExt)vkDevExtError;\n' elif x == 1: diff --git a/tests/framework/layer/test_layer.cpp b/tests/framework/layer/test_layer.cpp index 2764f665..14f0ebd6 100644 --- a/tests/framework/layer/test_layer.cpp +++ b/tests/framework/layer/test_layer.cpp @@ -277,6 +277,10 @@ VKAPI_ATTR VkResult VKAPI_CALL test_vkCreateInstance(const VkInstanceCreateInfo* } const VkInstanceCreateInfo* create_info_pointer = use_modified_create_info ? &instance_create_info : pCreateInfo; + if (layer.clobber_pInstance) { + memset(*pInstance, 0, 128); + } + // Continue call down the chain VkResult result = fpCreateInstance(create_info_pointer, pAllocator, pInstance); if (result != VK_SUCCESS) { @@ -401,6 +405,10 @@ VKAPI_ATTR VkResult VKAPI_CALL test_vkCreateDevice(VkPhysicalDevice physicalDevi // Advance the link info for the next element on the chain chain_info->u.pLayerInfo = chain_info->u.pLayerInfo->pNext; + if (layer.clobber_pDevice) { + memset(*pDevice, 0, 128); + } + VkResult result = fpCreateDevice(physicalDevice, pCreateInfo, pAllocator, pDevice); if (result != VK_SUCCESS) { return result; diff --git a/tests/framework/layer/test_layer.h b/tests/framework/layer/test_layer.h index 856f713f..d6d086c1 100644 --- a/tests/framework/layer/test_layer.h +++ b/tests/framework/layer/test_layer.h @@ -190,6 +190,11 @@ struct TestLayer { BUILDER_VALUE(TestLayer, bool, check_if_EnumDevExtProps_is_same_as_queried_function, false) + // Clober the data pointed to by pInstance to overwrite the magic value + BUILDER_VALUE(TestLayer, bool, clobber_pInstance, false) + // Clober the data pointed to by pDevice to overwrite the magic value + BUILDER_VALUE(TestLayer, bool, clobber_pDevice, false) + PFN_vkGetInstanceProcAddr next_vkGetInstanceProcAddr = VK_NULL_HANDLE; PFN_GetPhysicalDeviceProcAddr next_GetPhysicalDeviceProcAddr = VK_NULL_HANDLE; PFN_vkGetDeviceProcAddr next_vkGetDeviceProcAddr = VK_NULL_HANDLE; diff --git a/tests/loader_regression_tests.cpp b/tests/loader_regression_tests.cpp index 23e20135..bcc7b006 100644 --- a/tests/loader_regression_tests.cpp +++ b/tests/loader_regression_tests.cpp @@ -4069,3 +4069,80 @@ TEST(Layer, pfnNextGetInstanceProcAddr_should_not_return_layers_own_functions) { DeviceWrapper dev{inst}; dev.CheckCreate(phys_devs.at(0)); } + +TEST(Layer, LLP_LAYER_21) { + FrameworkEnvironment env{}; + env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2)).add_physical_device({}); + + env.add_implicit_layer(ManifestLayer{}.add_layer(ManifestLayer::LayerDescription{} + .set_name("implicit_layer_name") + .set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2) + .set_disable_environment("DISABLE_ME")), + "implicit_test_layer.json"); + env.get_test_layer(0).set_clobber_pInstance(true); + + InstWrapper inst{env.vulkan_functions}; + FillDebugUtilsCreateDetails(inst.create_info, env.debug_log); +#if defined(WIN32) +#if defined(_WIN64) + ASSERT_DEATH( + inst.CheckCreate(), + testing::ContainsRegex( + R"(terminator_CreateInstance: Instance pointer \(................\) has invalid MAGIC value 0x00000000. Instance value )" + R"(possibly corrupted by active layer \(Policy #LLP_LAYER_21\))")); +#else + ASSERT_DEATH( + inst.CheckCreate(), + testing::ContainsRegex( + R"(terminator_CreateInstance: Instance pointer \(........\) has invalid MAGIC value 0x00000000. Instance value )" + R"(possibly corrupted by active layer \(Policy #LLP_LAYER_21\))")); +#endif +#else + ASSERT_DEATH( + inst.CheckCreate(), + testing::ContainsRegex( + R"(terminator_CreateInstance: Instance pointer \(0x[0-9A-Fa-f]+\) has invalid MAGIC value 0x00000000. Instance value )" + R"(possibly corrupted by active layer \(Policy #LLP_LAYER_21\))")); +#endif +} + +TEST(Layer, LLP_LAYER_22) { + FrameworkEnvironment env{}; + env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2)).add_physical_device({}); + + env.add_implicit_layer(ManifestLayer{}.add_layer(ManifestLayer::LayerDescription{} + .set_name("implicit_layer_name") + .set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2) + .set_disable_environment("DISABLE_ME")), + "implicit_test_layer.json"); + env.get_test_layer(0).set_clobber_pDevice(true); + + InstWrapper inst{env.vulkan_functions}; + inst.create_info.add_extension("VK_EXT_debug_utils"); + inst.CheckCreate(); + + DebugUtilsWrapper log{inst}; + CreateDebugUtilsMessenger(log); + + DeviceWrapper dev{inst}; +#if defined(WIN32) +#if defined(_WIN64) + ASSERT_DEATH( + dev.CheckCreate(inst.GetPhysDev()), + testing::ContainsRegex( + R"(terminator_CreateDevice: Device pointer \(................\) has invalid MAGIC value 0x00000000. Device value )" + R"(possibly corrupted by active layer \(Policy #LLP_LAYER_22\))")); +#else + ASSERT_DEATH(dev.CheckCreate(inst.GetPhysDev()), + testing::ContainsRegex( + R"(terminator_CreateDevice: Device pointer \(........\) has invalid MAGIC value 0x00000000. Device value )" + R"(possibly corrupted by active layer \(Policy #LLP_LAYER_22\))")); +#endif +#else + ASSERT_DEATH( + dev.CheckCreate(inst.GetPhysDev()), + testing::ContainsRegex( + R"(terminator_CreateDevice: Device pointer \(0x[0-9A-Fa-f]+\) has invalid MAGIC value 0x00000000. Device value )" + R"(possibly corrupted by active layer \(Policy #LLP_LAYER_22\))")); +#endif +} -- cgit v1.2.3