[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>

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" );
 }