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

[Heinrich Schuchardt]

On 5/8/19 3:08 AM, Takahiro Akashi wrote:
> 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 at 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;

I think the goto usage here is in accordance with
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions

>>>> +	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;
>>>> +	}

These are the lines I moved down.

Regards

Heinrich

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