On 5/7/19 8:37 AM, Takahiro Akashi wrote:
> 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 <[email protected]>
>>>> ---
>>>> 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.
Good point. So the check must be after handling
EFI_OBJECT_TYPE_LOADED_IMAGE.
I will revise the patch.
Thanks for reviewing.
Best regards
Heinrich
>
> -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
>>>>
>>>
>>
>
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot
