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

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

On Wed, May 08, 2019 at 02:59:08AM +0200, Heinrich Schuchardt wrote:
> On 5/8/19 1:59 AM, Takahiro Akashi wrote:
> >On Tue, May 07, 2019 at 09:13:24PM +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>
> >>---
> >>v2
> >>	Images that are no yet started can be unloaded by calling Exit().
> >>	In this case they are not the current image. Move the test for
> >>	current down in the code.
> >>
> >>	A started driver that called Exit() should still be considered a
> >>	started image. Exit cannot be called by another image afterwards,
> >>	cf. UEFI SCT 2.6 (2017), 3.5.1 Exit(), 5.1.4.5.8 - 5.1.4.5.10.
> >>---
> >>  include/efi_loader.h              |  1 +
> >>  lib/efi_loader/efi_boottime.c     | 36 +++++++++++++++++++++++++------
> >>  lib/efi_loader/efi_image_loader.c |  2 ++
> >>  3 files changed, 33 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>index 3b50cd28ef..4e4cffa799 100644
> >>--- a/include/efi_loader.h
> >>+++ b/include/efi_loader.h
> >>@@ -234,6 +234,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 0385883ded..1ea96dab6c 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)
> >>-		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) {
> >>+		ret = EFI_INVALID_PARAMETER;
> >>  		goto out;
> >>+	}
> >>+
> >>+	/* Unloading of unstarted images */
> >>+	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;
> >>+	}
> >>+	/* A started image can only be unloaded it is the last one started. */
> >>+	if (image_handle != current_image) {
> >>+		ret = EFI_INVALID_PARAMETER;
> >>+		goto out;
> >>+	}
> >>
> >>  	/* 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);
> >
> >No change around efi_delete_image() and "goto" above?
> >
> 
> Do you see a bug?
> 
> A diff would help me to understand what you would like to change.

You said:
>> 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.
>
>Good point. So the check must be after handling
>EFI_OBJECT_TYPE_LOADED_IMAGE.
>
>I will revise the patch.

-Takahiro Akashi

> Best regards
> 
> Heinrich
>