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

[Heinrich Schuchardt]

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 at 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.

>
>> +		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(). In
principal a binary might have uninstalled its own loaded image protocol
so this call may fail.

>
>> +		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.

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