aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMichael Brown <mcb30@ipxe.org>2024-02-27 13:34:17 +0000
committerMichael Brown <mcb30@ipxe.org>2024-02-27 14:38:41 +0000
commit182ee909313fc60875d3f1741cd4a0bb7dfd15e1 (patch)
tree699730f570dbf392af6f6a4200a759862822bdd9
parent43e385091a36af34e495ac8c6595bddab55665bb (diff)
downloadipxe-182ee909313fc60875d3f1741cd4a0bb7dfd15e1.zip
ipxe-182ee909313fc60875d3f1741cd4a0bb7dfd15e1.tar.gz
ipxe-182ee909313fc60875d3f1741cd4a0bb7dfd15e1.tar.bz2
[efi] Work around broken boot services table manipulation by UEFI shim
The UEFI shim installs wrappers around several boot services functions before invoking its next stage bootloader, in an attempt to enforce its desired behaviour upon the aforementioned bootloader. For example, shim checks that the bootloader has either invoked StartImage() or has called into the "shim lock protocol" before allowing an ExitBootServices() call to proceed. When invoking a shim, iPXE will also install boot services function wrappers in order to work around assorted bugs in the UEFI shim code that would otherwise prevent it from being used to boot a kernel. For details on these workarounds, see commits 28184b7 ("[efi] Add support for executing images via a shim") and 5b43181 ("[efi] Support versions of shim that perform SBAT verification"). Using boot services function wrappers in this way is not intrinsically problematic, provided that wrappers are installed before starting the wrapped program, and uninstalled only after the wrapped program exits. This strict ordering requirement ensures that all layers of wrappers are called in the expected order, and that no calls are issued through a no-longer-valid function pointer. Unfortunately, the UEFI shim does not respect this strict ordering requirement, and will instead uninstall (and reinstall) its wrappers midway through the execution of the wrapped program. This leaves the wrapped program with an inconsistent view of the boot services table, leading to incorrect behaviour. This results in a boot failure when a first shim is used to boot iPXE, which then uses a second shim to boot a Linux kernel: - First shim installs StartImage() and ExitBootServices() wrappers - First shim invokes iPXE via its own PE loader - iPXE installs ExitBootServices() wrapper - iPXE invokes second shim via StartImage() At this point, the first shim's StartImage() wrapper will illegally uninstall its ExitBootServices() wrapper, without first checking that nothing else has modified the ExitBootServices function pointer. This effectively bypasses iPXE's own ExitBootServices() wrapper, which causes a boot failure since the code within that wrapper does not get called. A proper fix would be for shim to install its wrappers before starting the image and uninstall its wrappers only after the started image has exited. Instead of repeatedly uninstalling and reinstalling its wrappers while the wrapped program is running, shim should simply use a flag to keep track of whether or not it needs to modify the behaviour of the wrapped calls. Experience shows that there is unfortunately no point in trying to get a fix for this upstreamed into shim. We therefore work around the shim bug by removing our ExitBootServices() wrapper and moving the relevant code into our GetMemoryMap() wrapper. Signed-off-by: Michael Brown <mcb30@ipxe.org>
-rw-r--r--src/interface/efi/efi_shim.c92
1 files changed, 43 insertions, 49 deletions
diff --git a/src/interface/efi/efi_shim.c b/src/interface/efi/efi_shim.c
index a46d79d..d541951 100644
--- a/src/interface/efi/efi_shim.c
+++ b/src/interface/efi/efi_shim.c
@@ -112,9 +112,6 @@ struct image_tag efi_shim __image_tag = {
/** Original GetMemoryMap() function */
static EFI_GET_MEMORY_MAP efi_shim_orig_get_memory_map;
-/** Original ExitBootServices() function */
-static EFI_EXIT_BOOT_SERVICES efi_shim_orig_exit_boot_services;
-
/** Original SetVariable() function */
static EFI_SET_VARIABLE efi_shim_orig_set_variable;
@@ -161,49 +158,6 @@ static void efi_shim_unlock ( void ) {
}
/**
- * Wrap GetMemoryMap()
- *
- * @v len Memory map size
- * @v map Memory map
- * @v key Memory map key
- * @v desclen Descriptor size
- * @v descver Descriptor version
- * @ret efirc EFI status code
- */
-static EFIAPI EFI_STATUS efi_shim_get_memory_map ( UINTN *len,
- EFI_MEMORY_DESCRIPTOR *map,
- UINTN *key, UINTN *desclen,
- UINT32 *descver ) {
-
- /* Unlock shim */
- if ( ! efi_shim_require_loader )
- efi_shim_unlock();
-
- /* Hand off to original GetMemoryMap() */
- return efi_shim_orig_get_memory_map ( len, map, key, desclen,
- descver );
-}
-
-/**
- * Wrap ExitBootServices()
- *
- * @v handle Image handle
- * @v key Memory map key
- * @ret efirc EFI status code
- */
-static EFIAPI EFI_STATUS efi_shim_exit_boot_services ( EFI_HANDLE handle,
- UINTN key ) {
- EFI_RUNTIME_SERVICES *rs = efi_systab->RuntimeServices;
-
- /* Restore original runtime services functions */
- rs->SetVariable = efi_shim_orig_set_variable;
- rs->GetVariable = efi_shim_orig_get_variable;
-
- /* Hand off to original ExitBootServices() */
- return efi_shim_orig_exit_boot_services ( handle, key );
-}
-
-/**
* Wrap SetVariable()
*
* @v name Variable name
@@ -271,6 +225,47 @@ efi_shim_get_variable ( CHAR16 *name, EFI_GUID *guid, UINT32 *attrs,
}
/**
+ * Wrap GetMemoryMap()
+ *
+ * @v len Memory map size
+ * @v map Memory map
+ * @v key Memory map key
+ * @v desclen Descriptor size
+ * @v descver Descriptor version
+ * @ret efirc EFI status code
+ */
+static EFIAPI EFI_STATUS efi_shim_get_memory_map ( UINTN *len,
+ EFI_MEMORY_DESCRIPTOR *map,
+ UINTN *key, UINTN *desclen,
+ UINT32 *descver ) {
+ EFI_RUNTIME_SERVICES *rs = efi_systab->RuntimeServices;
+
+ /* Unlock shim */
+ if ( ! efi_shim_require_loader )
+ efi_shim_unlock();
+
+ /* Uninstall runtime services wrappers, if still installed */
+ if ( rs->SetVariable == efi_shim_set_variable ) {
+ rs->SetVariable = efi_shim_orig_set_variable;
+ DBGC ( &efi_shim, "SHIM uninstalled SetVariable() wrapper\n" );
+ } else if ( rs->SetVariable != efi_shim_orig_set_variable ) {
+ DBGC ( &efi_shim, "SHIM could not uninstall SetVariable() "
+ "wrapper!\n" );
+ }
+ if ( rs->GetVariable == efi_shim_get_variable ) {
+ rs->GetVariable = efi_shim_orig_get_variable;
+ DBGC ( &efi_shim, "SHIM uninstalled GetVariable() wrapper\n" );
+ } else if ( rs->GetVariable != efi_shim_orig_get_variable ) {
+ DBGC ( &efi_shim, "SHIM could not uninstall GetVariable() "
+ "wrapper!\n" );
+ }
+
+ /* Hand off to original GetMemoryMap() */
+ return efi_shim_orig_get_memory_map ( len, map, key, desclen,
+ descver );
+}
+
+/**
* Inhibit use of PXE base code
*
* @v handle EFI handle
@@ -373,15 +368,14 @@ int efi_shim_install ( struct image *shim, EFI_HANDLE handle,
/* Record original boot and runtime services functions */
efi_shim_orig_get_memory_map = bs->GetMemoryMap;
- efi_shim_orig_exit_boot_services = bs->ExitBootServices;
efi_shim_orig_set_variable = rs->SetVariable;
efi_shim_orig_get_variable = rs->GetVariable;
/* Wrap relevant boot and runtime services functions */
bs->GetMemoryMap = efi_shim_get_memory_map;
- bs->ExitBootServices = efi_shim_exit_boot_services;
rs->SetVariable = efi_shim_set_variable;
rs->GetVariable = efi_shim_get_variable;
+ DBGC ( &efi_shim, "SHIM installed wrappers\n" );
return 0;
}
@@ -396,7 +390,7 @@ void efi_shim_uninstall ( void ) {
/* Restore original boot and runtime services functions */
bs->GetMemoryMap = efi_shim_orig_get_memory_map;
- bs->ExitBootServices = efi_shim_orig_exit_boot_services;
rs->SetVariable = efi_shim_orig_set_variable;
rs->GetVariable = efi_shim_orig_get_variable;
+ DBGC ( &efi_shim, "SHIM uninstalled wrappers\n" );
}