From 25a3d3acabeec97c4a4cb8ed8fb90853e8a195c3 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Thu, 8 Jun 2023 11:29:07 +0100 Subject: [efi] Veto the VMware UefiPxeBcDxe driver The EDK2 UefiPxeBcDxe driver includes some remarkably convoluted and unsafe logic in its driver binding protocol Start() and Stop() methods in order to support a pair of nominally independent driver binding protocols (one for IPv4, one for IPv6) sharing a single dynamically allocated data structure. This PXEBC_PRIVATE_DATA structure is installed as a dummy protocol on the NIC handle in order to allow both IPv4 and IPv6 driver binding protocols to locate it as needed. The error handling code path in the UefiPxeBcDxe driver's Start() method may attempt to uninstall the dummy protocol but fail to do so. This failure is ignored and the containing memory is subsequently freed anyway. On the next invocation of the driver binding protocol, it will find and use this already freed block of memory. At some point another memory allocation will occur, the PXEBC_PRIVATE_DATA structure will be corrupted, and some undefined behaviour will occur. The UEFI firmware used in VMware ESX 8 includes some proprietary changes which attempt to install copies of the EFI_LOAD_FILE_PROTOCOL and EFI_PXE_BASE_CODE_PROTOCOL instances from the IPv4 child handle onto the NIC handle (along with a VMware-specific protocol with GUID 5190120d-453b-4d48-958d-f0bab3bc2161 and a NULL instance pointer). This will inevitably fail with iPXE, since the NIC handle already includes an EFI_LOAD_FILE_PROTOCOL instance. These VMware proprietary changes end up triggering the unsafe error handling code path described above. The typical symptom is that an attempt to exit from iPXE back to the UEFI firmware will crash the VM with a General Protection fault from within the UefiPxeBcDxe driver: this happens when the UefiPxeBcDxe driver's Stop() method attempts to call through a function pointer in the (freed) PXEBC_PRIVATE_DATA structure, but the function pointer has by then been overwritten by UCS-2 character data from an unrelated memory allocation. Work around this failure by adding the VMware UefiPxeBcDxe driver to the driver veto list. Signed-off-by: Michael Brown --- src/interface/efi/efi_veto.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) (limited to 'src/interface/efi/efi_veto.c') diff --git a/src/interface/efi/efi_veto.c b/src/interface/efi/efi_veto.c index b616539d3..19e529af6 100644 --- a/src/interface/efi/efi_veto.c +++ b/src/interface/efi/efi_veto.c @@ -435,6 +435,37 @@ efi_veto_hp_xhci ( EFI_DRIVER_BINDING_PROTOCOL *binding __unused, return 0; } +/** + * Veto VMware UefiPxeBcDxe driver + * + * @v binding Driver binding protocol + * @v loaded Loaded image protocol + * @v wtf Component name protocol, if present + * @v manufacturer Manufacturer name, if present + * @v name Driver name, if present + * @ret vetoed Driver is to be vetoed + */ +static int +efi_veto_vmware_uefipxebc ( EFI_DRIVER_BINDING_PROTOCOL *binding __unused, + EFI_LOADED_IMAGE_PROTOCOL *loaded __unused, + EFI_COMPONENT_NAME_PROTOCOL *wtf __unused, + const char *manufacturer, const CHAR16 *name ) { + static const CHAR16 uefipxebc[] = L"UEFI PXE Base Code Driver"; + static const char *vmware = "VMware, Inc."; + + /* Check manufacturer and driver name */ + if ( ! manufacturer ) + return 0; + if ( ! name ) + return 0; + if ( strcmp ( manufacturer, vmware ) != 0 ) + return 0; + if ( memcmp ( name, uefipxebc, sizeof ( uefipxebc ) ) != 0 ) + return 0; + + return 1; +} + /** Driver vetoes */ static struct efi_veto efi_vetoes[] = { { @@ -445,6 +476,10 @@ static struct efi_veto efi_vetoes[] = { .name = "HP Xhci", .veto = efi_veto_hp_xhci, }, + { + .name = "VMware UefiPxeBc", + .veto = efi_veto_vmware_uefipxebc, + }, }; /** -- cgit v1.2.3-55-g7522 From 9a118322a025986b350343daf9d55882d3238327 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Thu, 22 Jun 2023 23:20:37 +0100 Subject: [efi] Show manufacturer in veto debug output Simplify the process of adding new entries to the veto list by including the manufacturer name within the standard debug output. Signed-off-by: Michael Brown --- src/interface/efi/efi_veto.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src/interface/efi/efi_veto.c') diff --git a/src/interface/efi/efi_veto.c b/src/interface/efi/efi_veto.c index 19e529af6..e4791272b 100644 --- a/src/interface/efi/efi_veto.c +++ b/src/interface/efi/efi_veto.c @@ -617,6 +617,7 @@ void efi_veto ( void ) { /* Get manufacturer name */ fetch_string_setting_copy ( NULL, &manufacturer_setting, &manufacturer ); + DBGC ( &efi_vetoes, "EFIVETO manufacturer is \"%s\"\n", manufacturer ); /* Unload any vetoed drivers */ for ( i = 0 ; i < num_drivers ; i++ ) { -- cgit v1.2.3-55-g7522 From c832580f197dac013edb72ce031570d66a9448f6 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 23 Jun 2023 16:05:42 +0100 Subject: [efi] Pass more detailed driver information to veto methods Pass the driver binding handle, the driver binding protocol instance, the image handle, and the loaded image protocol instance to all veto methods. Signed-off-by: Michael Brown --- src/interface/efi/efi_veto.c | 94 +++++++++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 36 deletions(-) (limited to 'src/interface/efi/efi_veto.c') diff --git a/src/interface/efi/efi_veto.c b/src/interface/efi/efi_veto.c index e4791272b..79c157571 100644 --- a/src/interface/efi/efi_veto.c +++ b/src/interface/efi/efi_veto.c @@ -37,8 +37,8 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL ); * */ -/** A driver veto */ -struct efi_veto { +/** A driver veto candidate */ +struct efi_veto_candidate { /** Veto name (for debugging) */ const char *name; /** @@ -57,14 +57,27 @@ struct efi_veto { const char *manufacturer, const CHAR16 *name ); }; +/** A driver veto */ +struct efi_veto { + /** Driver binding handle */ + EFI_HANDLE driver; + /** Driving binding protocol */ + EFI_DRIVER_BINDING_PROTOCOL *binding; + /** Image handle */ + EFI_HANDLE image; + /** Loaded image protocol */ + EFI_LOADED_IMAGE_PROTOCOL *loaded; +}; + /** * Unload an EFI driver * - * @v driver Driver binding handle + * @v veto Driver veto * @ret rc Return status code */ -static int efi_veto_unload ( EFI_HANDLE driver ) { +static int efi_veto_unload ( struct efi_veto *veto ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; + EFI_HANDLE driver = veto->driver; EFI_STATUS efirc; int rc; @@ -82,11 +95,12 @@ static int efi_veto_unload ( EFI_HANDLE driver ) { /** * Disconnect an EFI driver from all handles * - * @v driver Driver binding handle + * @v veto Driver veto * @ret rc Return status code */ -static int efi_veto_disconnect ( EFI_HANDLE driver ) { +static int efi_veto_disconnect ( struct efi_veto *veto ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; + EFI_HANDLE driver = veto->driver; EFI_HANDLE *handles; EFI_HANDLE handle; UINTN count; @@ -131,11 +145,12 @@ static int efi_veto_disconnect ( EFI_HANDLE driver ) { /** * Uninstall an EFI driver binding protocol * - * @v driver Driver binding handle + * @v veto Driver veto * @ret rc Return status code */ -static int efi_veto_uninstall ( EFI_HANDLE driver ) { +static int efi_veto_uninstall ( struct efi_veto *veto ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; + EFI_HANDLE driver = veto->driver; union { EFI_DRIVER_BINDING_PROTOCOL *binding; void *interface; @@ -178,14 +193,15 @@ static int efi_veto_uninstall ( EFI_HANDLE driver ) { /** * Close protocol on handle potentially opened by an EFI driver * - * @v driver Driver binding handle + * @v veto Driver veto * @v handle Potentially opened handle * @v protocol Opened protocol * @ret rc Return status code */ -static int efi_veto_close_protocol ( EFI_HANDLE driver, EFI_HANDLE handle, +static int efi_veto_close_protocol ( struct efi_veto *veto, EFI_HANDLE handle, EFI_GUID *protocol ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; + EFI_HANDLE driver = veto->driver; EFI_OPEN_PROTOCOL_INFORMATION_ENTRY *openers; EFI_OPEN_PROTOCOL_INFORMATION_ENTRY *opener; EFI_HANDLE controller; @@ -235,12 +251,13 @@ static int efi_veto_close_protocol ( EFI_HANDLE driver, EFI_HANDLE handle, /** * Close handle potentially opened by an EFI driver * - * @v driver Driver binding handle + * @v veto Driver veto * @v handle Potentially opened handle * @ret rc Return status code */ -static int efi_veto_close_handle ( EFI_HANDLE driver, EFI_HANDLE handle ) { +static int efi_veto_close_handle ( struct efi_veto *veto, EFI_HANDLE handle ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; + EFI_HANDLE driver = veto->driver; EFI_GUID **protocols; UINTN count; unsigned int i; @@ -260,7 +277,7 @@ static int efi_veto_close_handle ( EFI_HANDLE driver, EFI_HANDLE handle ) { /* Close each protocol */ for ( i = 0 ; i < count ; i++ ) { - if ( ( rc = efi_veto_close_protocol ( driver, handle, + if ( ( rc = efi_veto_close_protocol ( veto, handle, protocols[i] ) ) != 0 ) goto err_close; } @@ -277,11 +294,12 @@ static int efi_veto_close_handle ( EFI_HANDLE driver, EFI_HANDLE handle ) { /** * Close all remaining handles opened by an EFI driver * - * @v driver Driver binding handle + * @v veto Driver veto * @ret rc Return status code */ -static int efi_veto_close ( EFI_HANDLE driver ) { +static int efi_veto_close ( struct efi_veto *veto ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; + EFI_HANDLE driver = veto->driver; EFI_HANDLE *handles; UINTN count; unsigned int i; @@ -299,8 +317,7 @@ static int efi_veto_close ( EFI_HANDLE driver ) { /* Close each handle */ for ( i = 0 ; i < count ; i++ ) { - if ( ( rc = efi_veto_close_handle ( driver, - handles[i] ) ) != 0 ) + if ( ( rc = efi_veto_close_handle ( veto, handles[i] ) ) != 0 ) goto err_close; } @@ -318,22 +335,23 @@ static int efi_veto_close ( EFI_HANDLE driver ) { /** * Terminate an EFI driver with extreme prejudice * - * @v driver Driver binding handle + * @v veto Driver veto * @ret rc Return status code */ -static int efi_veto_destroy ( EFI_HANDLE driver ) { +static int efi_veto_destroy ( struct efi_veto *veto ) { + EFI_HANDLE driver = veto->driver; int rc; /* Disconnect driver from all handles */ - if ( ( rc = efi_veto_disconnect ( driver ) ) != 0 ) + if ( ( rc = efi_veto_disconnect ( veto ) ) != 0 ) return rc; /* Uninstall driver binding protocol */ - if ( ( rc = efi_veto_uninstall ( driver ) ) != 0 ) + if ( ( rc = efi_veto_uninstall ( veto ) ) != 0 ) return rc; /* Close any remaining opened handles */ - if ( ( rc = efi_veto_close ( driver ) ) != 0 ) + if ( ( rc = efi_veto_close ( veto ) ) != 0 ) return rc; DBGC ( driver, "EFIVETO %s forcibly removed\n", @@ -344,18 +362,18 @@ static int efi_veto_destroy ( EFI_HANDLE driver ) { /** * Veto an EFI driver * - * @v driver Driver binding handle + * @v veto Driver veto * @ret rc Return status code */ -static int efi_veto_driver ( EFI_HANDLE driver ) { +static int efi_veto_driver ( struct efi_veto *veto ) { int rc; /* Try gracefully unloading the driver */ - if ( ( rc = efi_veto_unload ( driver ) ) == 0 ) + if ( ( rc = efi_veto_unload ( veto ) ) == 0 ) return 0; /* If that fails, use a hammer */ - if ( ( rc = efi_veto_destroy ( driver ) ) == 0 ) + if ( ( rc = efi_veto_destroy ( veto ) ) == 0 ) return 0; return rc; @@ -467,7 +485,7 @@ efi_veto_vmware_uefipxebc ( EFI_DRIVER_BINDING_PROTOCOL *binding __unused, } /** Driver vetoes */ -static struct efi_veto efi_vetoes[] = { +static struct efi_veto_candidate efi_vetoes[] = { { .name = "Ip4Config", .veto = efi_veto_ip4config, @@ -487,11 +505,11 @@ static struct efi_veto efi_vetoes[] = { * * @v driver Driver binding handle * @v manufacturer Manufacturer name, if present - * @ret veto Driver veto, or NULL + * @ret veto Driver veto to fill in * @ret rc Return status code */ static int efi_veto_find ( EFI_HANDLE driver, const char *manufacturer, - struct efi_veto **veto ) { + struct efi_veto *veto ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; union { EFI_DRIVER_BINDING_PROTOCOL *binding; @@ -515,7 +533,7 @@ static int efi_veto_find ( EFI_HANDLE driver, const char *manufacturer, efi_handle_name ( driver ) ); /* Mark as not vetoed */ - *veto = NULL; + memset ( veto, 0, sizeof ( *veto ) ); /* Open driver binding protocol */ if ( ( efirc = bs->OpenProtocol ( @@ -567,7 +585,13 @@ static int efi_veto_find ( EFI_HANDLE driver, const char *manufacturer, sizeof ( efi_vetoes[0] ) ) ; i++ ) { if ( efi_vetoes[i].veto ( binding.binding, loaded.loaded, wtf.wtf, manufacturer, name ) ) { - *veto = &efi_vetoes[i]; + DBGC ( driver, "EFIVETO %s is vetoed (%s)\n", + efi_handle_name ( driver ), + efi_vetoes[i].name ); + veto->driver = driver; + veto->binding = binding.binding; + veto->image = image; + veto->loaded = loaded.loaded; break; } } @@ -595,7 +619,7 @@ static int efi_veto_find ( EFI_HANDLE driver, const char *manufacturer, */ void efi_veto ( void ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; - struct efi_veto *veto; + struct efi_veto veto; EFI_HANDLE *drivers; EFI_HANDLE driver; UINTN num_drivers; @@ -629,11 +653,9 @@ void efi_veto ( void ) { efi_handle_name ( driver ), strerror ( rc ) ); continue; } - if ( ! veto ) + if ( ! veto.driver ) continue; - DBGC ( driver, "EFIVETO %s is vetoed (%s)\n", - efi_handle_name ( driver ), veto->name ); - if ( ( rc = efi_veto_driver ( driver ) ) != 0 ) { + if ( ( rc = efi_veto_driver ( &veto ) ) != 0 ) { DBGC ( driver, "EFIVETO %s could not veto: %s\n", efi_handle_name ( driver ), strerror ( rc ) ); } -- cgit v1.2.3-55-g7522 From f0b1025503205515d5b105c37774596623481242 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 23 Jun 2023 16:08:25 +0100 Subject: [efi] Unload vetoed drivers by image handle rather than driver handle In most cases, the driver handle will be the image handle itself. However, this is not required by the UEFI specification, and some images will install multiple driver binding handles. Use the image handle (extracted from the driver binding protocol instance) when attempting to unload the driver's image. Signed-off-by: Michael Brown --- src/interface/efi/efi_veto.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'src/interface/efi/efi_veto.c') diff --git a/src/interface/efi/efi_veto.c b/src/interface/efi/efi_veto.c index 79c157571..3e0806e01 100644 --- a/src/interface/efi/efi_veto.c +++ b/src/interface/efi/efi_veto.c @@ -78,14 +78,17 @@ struct efi_veto { static int efi_veto_unload ( struct efi_veto *veto ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; EFI_HANDLE driver = veto->driver; + EFI_HANDLE image = veto->image; EFI_STATUS efirc; int rc; /* Unload the driver */ - if ( ( efirc = bs->UnloadImage ( driver ) ) != 0 ) { + if ( ( efirc = bs->UnloadImage ( image ) ) != 0 ) { rc = -EEFI ( efirc ); - DBGC ( driver, "EFIVETO %s could not unload: %s\n", - efi_handle_name ( driver ), strerror ( rc ) ); + DBGC ( driver, "EFIVETO %s could not unload", + efi_handle_name ( driver ) ); + DBGC ( driver, " %s: %s\n", efi_handle_name ( image ), + strerror ( rc ) ); return rc; } -- cgit v1.2.3-55-g7522 From f8a0d1c0b8cfd7b04bdc9a006c52434661afd06a Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 23 Jun 2023 16:12:01 +0100 Subject: [efi] Check for protocols opened by vetoed driver and image handles The UEFI specification states that the AgentHandle may be either the driving binding protocol handle or the image handle. Check for both handles when searching for stale handles to be forcibly closed on behalf of a vetoed driver. Signed-off-by: Michael Brown --- src/interface/efi/efi_veto.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/interface/efi/efi_veto.c') diff --git a/src/interface/efi/efi_veto.c b/src/interface/efi/efi_veto.c index 3e0806e01..bc19a0091 100644 --- a/src/interface/efi/efi_veto.c +++ b/src/interface/efi/efi_veto.c @@ -205,6 +205,7 @@ static int efi_veto_close_protocol ( struct efi_veto *veto, EFI_HANDLE handle, EFI_GUID *protocol ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; EFI_HANDLE driver = veto->driver; + EFI_HANDLE image = veto->image; EFI_OPEN_PROTOCOL_INFORMATION_ENTRY *openers; EFI_OPEN_PROTOCOL_INFORMATION_ENTRY *opener; EFI_HANDLE controller; @@ -227,8 +228,10 @@ static int efi_veto_close_protocol ( struct efi_veto *veto, EFI_HANDLE handle, /* Close anything opened by this driver */ for ( i = 0 ; i < count ; i++ ) { opener = &openers[i]; - if ( opener->AgentHandle != driver ) + if ( ( opener->AgentHandle != driver ) && + ( opener->AgentHandle != image ) ) { continue; + } controller = opener->ControllerHandle; DBGC_EFI_OPENER ( driver, handle, protocol, opener ); if ( ( efirc = bs->CloseProtocol ( handle, protocol, driver, -- cgit v1.2.3-55-g7522 From ae435cb4cc78183814f867686d278885db2988f8 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 23 Jun 2023 16:27:47 +0100 Subject: [efi] Process veto objects in reverse order of enumeration While not guaranteed by the UEFI specification, the enumeration of handles, protocols, and openers will generally return results in order of creation. Processing these objects in reverse order (as is already done when calling DisconnectController() on the list of all handles) will generally therefore perform the forcible uninstallation operations in reverse order of object creation, which minimises the number of implicit operations performed (e.g. when disconnecting a controller that itself still has existent child controllers). Signed-off-by: Michael Brown --- src/interface/efi/efi_veto.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) (limited to 'src/interface/efi/efi_veto.c') diff --git a/src/interface/efi/efi_veto.c b/src/interface/efi/efi_veto.c index bc19a0091..a3b60d65f 100644 --- a/src/interface/efi/efi_veto.c +++ b/src/interface/efi/efi_veto.c @@ -227,7 +227,7 @@ static int efi_veto_close_protocol ( struct efi_veto *veto, EFI_HANDLE handle, /* Close anything opened by this driver */ for ( i = 0 ; i < count ; i++ ) { - opener = &openers[i]; + opener = &openers[ count - i - 1 ]; if ( ( opener->AgentHandle != driver ) && ( opener->AgentHandle != image ) ) { continue; @@ -265,6 +265,7 @@ static int efi_veto_close_handle ( struct efi_veto *veto, EFI_HANDLE handle ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; EFI_HANDLE driver = veto->driver; EFI_GUID **protocols; + EFI_GUID *protocol; UINTN count; unsigned int i; EFI_STATUS efirc; @@ -283,8 +284,9 @@ static int efi_veto_close_handle ( struct efi_veto *veto, EFI_HANDLE handle ) { /* Close each protocol */ for ( i = 0 ; i < count ; i++ ) { + protocol = protocols[ count - i - 1]; if ( ( rc = efi_veto_close_protocol ( veto, handle, - protocols[i] ) ) != 0 ) + protocol ) ) != 0 ) goto err_close; } @@ -307,6 +309,7 @@ static int efi_veto_close ( struct efi_veto *veto ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; EFI_HANDLE driver = veto->driver; EFI_HANDLE *handles; + EFI_HANDLE handle; UINTN count; unsigned int i; EFI_STATUS efirc; @@ -323,7 +326,8 @@ static int efi_veto_close ( struct efi_veto *veto ) { /* Close each handle */ for ( i = 0 ; i < count ; i++ ) { - if ( ( rc = efi_veto_close_handle ( veto, handles[i] ) ) != 0 ) + handle = handles[ count - i - 1 ]; + if ( ( rc = efi_veto_close_handle ( veto, handle ) ) != 0 ) goto err_close; } @@ -628,7 +632,7 @@ void efi_veto ( void ) { struct efi_veto veto; EFI_HANDLE *drivers; EFI_HANDLE driver; - UINTN num_drivers; + UINTN count; unsigned int i; char *manufacturer; EFI_STATUS efirc; @@ -637,7 +641,7 @@ void efi_veto ( void ) { /* Locate all driver binding protocol handles */ if ( ( efirc = bs->LocateHandleBuffer ( ByProtocol, &efi_driver_binding_protocol_guid, - NULL, &num_drivers, &drivers ) ) != 0 ) { + NULL, &count, &drivers ) ) != 0 ) { rc = -EEFI ( efirc ); DBGC ( &efi_vetoes, "EFIVETO could not list all drivers: " "%s\n", strerror ( rc ) ); @@ -650,8 +654,8 @@ void efi_veto ( void ) { DBGC ( &efi_vetoes, "EFIVETO manufacturer is \"%s\"\n", manufacturer ); /* Unload any vetoed drivers */ - for ( i = 0 ; i < num_drivers ; i++ ) { - driver = drivers[i]; + for ( i = 0 ; i < count ; i++ ) { + driver = drivers[ count - i - 1 ]; if ( ( rc = efi_veto_find ( driver, manufacturer, &veto ) ) != 0 ) { DBGC ( driver, "EFIVETO %s could not determine " -- cgit v1.2.3-55-g7522