[U-Boot] [PATCH 4/4] efi_loader: unload applications upon Exit()

[Takahiro Akashi <takahiro.akashi@linaro.org>]

On Tue, May 07, 2019 at 07:50:48AM +0200, Heinrich Schuchardt wrote:
> On 5/7/19 6:39 AM, Takahiro Akashi wrote:
> >On Sat, May 04, 2019 at 10:36:36AM +0200, Heinrich Schuchardt wrote:
> >>Implement unloading of images in the Exit() boot services:
> >>
> >>* unload images that are not yet started,
> >>* unload started applications,
> >>* unload drivers returning an error.
> >>
> >>Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>---
> >>  include/efi_loader.h              |  1 +
> >>  lib/efi_loader/efi_boottime.c     | 34 ++++++++++++++++++++++++++-----
> >>  lib/efi_loader/efi_image_loader.c |  2 ++
> >>  3 files changed, 32 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>index c2a449e5b6..d73c89ac26 100644
> >>--- a/include/efi_loader.h
> >>+++ b/include/efi_loader.h
> >>@@ -237,6 +237,7 @@ struct efi_loaded_image_obj {
> >>  	struct jmp_buf_data exit_jmp;
> >>  	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
> >>  				     struct efi_system_table *st);
> >>+	u16 image_type;
> >>  };
> >>
> >>  /**
> >>diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >>index bbcd66caa6..51e0bb2fd5 100644
> >>--- a/lib/efi_loader/efi_boottime.c
> >>+++ b/lib/efi_loader/efi_boottime.c
> >>@@ -13,6 +13,7 @@
> >>  #include <linux/libfdt_env.h>
> >>  #include <u-boot/crc.h>
> >>  #include <bootm.h>
> >>+#include <pe.h>
> >>  #include <watchdog.h>
> >>
> >>  DECLARE_GLOBAL_DATA_PTR;
> >>@@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >>  	 *	 image protocol.
> >>  	 */
> >>  	efi_status_t ret;
> >>-	void *info;
> >>+	struct efi_loaded_image *loaded_image_protocol;
> >>  	struct efi_loaded_image_obj *image_obj =
> >>  		(struct efi_loaded_image_obj *)image_handle;
> >>
> >>@@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >>  		  exit_data_size, exit_data);
> >>
> >>  	/* Check parameters */
> >>-	if (image_handle != current_image)
> >>+	if (image_handle != current_image) {
> >
> >Does this check make sense even for a driver?
> 
> The check is prescribed in the UEFI specification. See the heading
> "Status Codes Returned".
> 
> Multiple binaries may be in status started. If a child image (which may
> be a driver) calls Exit() with the parent image handle this might cause
> fancy problems.
> 
> The lifetime of a driver is LoadImage(), StartImage(), Exit(),
> UnloadImage(). Typically between StartImage() and Exit() it will install
> its protocols and between Exit() and UnloadImage() wait for other
> binaries to call the protocols.

If this is true, that will be fine for a driver.
But what about 'not started' application? In "Status Codes Returned" of
Exit(), it says
  EFI_SUCCESS
  The image specified by ImageHandle was unloaded. This condition only
  occurs for images that have been loaded with LoadImage() but have not
  been started with StartImage().
In this case, the caller of Exit() is not an application.

> >
> >>+		ret = EFI_INVALID_PARAMETER;
> >>  		goto out;
> >>+	}
> >>  	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
> >>-					 &info, NULL, NULL,
> >>+					 (void **)&loaded_image_protocol,
> >>+					 NULL, NULL,
> >>  					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
> >>-	if (ret != EFI_SUCCESS)
> >>+	if (ret != EFI_SUCCESS) {
> >
> >Is this check necessary even when 'header.type' is introduced?
> 
> I am passing the loaded image protocol to efi_delete_handle().

I don't think that it can be a critical reason.

> In
> principal a binary might have uninstalled its own loaded image protocol
> so this call may fail.

If we admit this possibility, there will be bunch of fragile code elsewhere.

> >
> >>+		ret = EFI_INVALID_PARAMETER;
> >>  		goto out;
> >>+	}
> >>+
> >>+	/* Unloading of unstarted images */
> >
> >'Unload' sounds odd when we call efi_delete_image(), doesn't it?
> >
> >>+	switch (image_obj->header.type) {
> >>+	case EFI_OBJECT_TYPE_STARTED_IMAGE:
> >>+		break;
> >>+	case EFI_OBJECT_TYPE_LOADED_IMAGE:
> >>+		efi_delete_image(image_obj, loaded_image_protocol);
> >>+		ret = EFI_SUCCESS;
> >>+		goto out;
> >>+	default:
> >>+		/* Handle does not refer to loaded image */
> >>+		ret = EFI_INVALID_PARAMETER;
> >>+		goto out;
> >>+	}
> >>+	image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
> >>
> >>  	/* Exit data is only foreseen in case of failure. */
> >>  	if (exit_status != EFI_SUCCESS) {
> >>@@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >>  		if (ret != EFI_SUCCESS)
> >>  			EFI_PRINT("%s: out of memory\n", __func__);
> >>  	}
> >>+	if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
> >>+	    exit_status != EFI_SUCCESS)
> >>+		efi_delete_image(image_obj, loaded_image_protocol);
> >
> >Why not merge those two efi_delete_image()?
> 
> I went for readable code. The if statement catching all cases was
> unreadable to me.

For me, your code is much unreadable.
Moreover, I remember that you have said, in a review of my patch, that
we should use "goto" only in error cases.

-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> >
> >-Takahiro Akashi
> >
> >
> >>  	/* Make sure entry/exit counts for EFI world cross-overs match */
> >>  	EFI_EXIT(exit_status);
> >>@@ -2837,7 +2861,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >>
> >>  	panic("EFI application exited");
> >>  out:
> >>-	return EFI_EXIT(EFI_INVALID_PARAMETER);
> >>+	return EFI_EXIT(ret);
> >>  }
> >>
> >>  /**
> >>diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> >>index f8092b6202..13541cfa7a 100644
> >>--- a/lib/efi_loader/efi_image_loader.c
> >>+++ b/lib/efi_loader/efi_image_loader.c
> >>@@ -273,6 +273,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
> >>  		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
> >>  		image_base = opt->ImageBase;
> >>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> >>+		handle->image_type = opt->Subsystem;
> >>  		efi_reloc = efi_alloc(virt_size,
> >>  				      loaded_image_info->image_code_type);
> >>  		if (!efi_reloc) {
> >>@@ -288,6 +289,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
> >>  		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
> >>  		image_base = opt->ImageBase;
> >>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> >>+		handle->image_type = opt->Subsystem;
> >>  		efi_reloc = efi_alloc(virt_size,
> >>  				      loaded_image_info->image_code_type);
> >>  		if (!efi_reloc) {
> >>--
> >>2.20.1
> >>
> >
>