summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorCharles Giessen <charles@lunarg.com>2023-08-22 15:49:17 -0600
committerCharles Giessen <46324611+charles-lunarg@users.noreply.github.com>2023-09-12 14:23:26 -0600
commit1e09b25e96fbcb8ab7b40a07ef21afb89ffe54f7 (patch)
tree5f422c5cd44f91df6086a69960237ff3f110243b
parent1c827fd79cee8a5248bb3d55f9da26abca89c7db (diff)
downloadVulkan-Loader-1e09b25e96fbcb8ab7b40a07ef21afb89ffe54f7.tar.gz
Vulkan-Loader-1e09b25e96fbcb8ab7b40a07ef21afb89ffe54f7.tar.bz2
Vulkan-Loader-1e09b25e96fbcb8ab7b40a07ef21afb89ffe54f7.zip
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.
-rw-r--r--docs/LoaderLayerInterface.md16
-rw-r--r--loader/generated/vk_loader_extensions.c2
-rw-r--r--loader/loader.c13
-rw-r--r--loader/trampoline.c32
-rw-r--r--scripts/loader_extension_generator.py2
-rw-r--r--tests/framework/layer/test_layer.cpp8
-rw-r--r--tests/framework/layer/test_layer.h5
-rw-r--r--tests/loader_regression_tests.cpp77
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</a>.
<td>Yes</td>
<td><small>N/A</small></td>
</tr>
+ <tr>
+ <td><small><b>LLP_LAYER_22</b></small></td>
+ <td>During <i>vkCreateDevice</i>, a layer <b>must not</b> modify the
+ <i>pDevice</i> pointer during prior to calling down to the lower
+ layers.<br/>
+ This is because the loader passes information in this pointer that is
+ necessary for the initialization code in the loader's terminator
+ function.<br/>
+ Instead, if the layer is overriding the <i>pDevice</i> pointer, it
+ <b>must</b> do so only after the call to the lower layers returns.
+ </td>
+ <td>The loader will likely crash.</td>
+ <td>No</td>
+ <td>Yes</td>
+ <td><small>N/A</small></td>
+ </tr>
</table>
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
+}