[PATCH] efi_loader: make efi_delete_handle() follow the EFI spec
Heinrich Schuchardt
xypron.glpk at gmx.de
Sun Jul 9 11:28:13 CEST 2023
On 6/26/23 12:04, Ilias Apalodimas wrote:
> The EFI doesn't allow removal of handles, unless all hosted protocols
> are cleanly removed. Our efi_delete_handle() is a bit intrusive.
> Although it does try to delete protocols before removing a handle,
> it doesn't care if that fails. Instead it only returns an error if the
> handle is invalid. On top of that none of the callers of that function
> check the return code.
>
> So let's rewrite this in a way that fits the EFI spec better. Instead
> of forcing the handle removal, gracefully uninstall all the handle
> protocols. According to the EFI spec when the last protocol is removed
> the handle will be deleted. Also switch all the callers and check the
> return code. Some callers can't do anything useful apart from reporting
> an error. The disk related functions on the other hand, can prevent a
> medium that is being used by EFI from removal.
>
> The only function that doesn't check the result is efi_delete_image().
> But that function needs a bigger rework anyway, so we can clean it up in
> the future
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
> Heinrich this needs to be applied on top of
> https://lore.kernel.org/u-boot/20230620061932.113292-1-ilias.apalodimas@linaro.org/
>
> cmd/bootefi.c | 19 ++++---------------
> include/efi_loader.h | 2 +-
> lib/efi_driver/efi_uclass.c | 13 ++++++-------
> lib/efi_loader/efi_boottime.c | 26 ++++++++++++++++++--------
> lib/efi_loader/efi_disk.c | 17 +++++++++++++----
> 5 files changed, 42 insertions(+), 35 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 8aa15a64c836..60b9e950ffdc 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -606,20 +606,6 @@ failure:
> return ret;
> }
>
> -/**
> - * bootefi_run_finish() - finish up after running an EFI test
> - *
> - * @loaded_image_info: Pointer to a struct which holds the loaded image info
> - * @image_obj: Pointer to a struct which holds the loaded image object
> - */
> -static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> - struct efi_loaded_image *loaded_image_info)
> -{
> - efi_restore_gd();
> - free(loaded_image_info->load_options);
> - efi_delete_handle(&image_obj->header);
> -}
> -
> /**
> * do_efi_selftest() - execute EFI selftest
> *
> @@ -638,7 +624,10 @@ static int do_efi_selftest(void)
>
> /* Execute the test */
> ret = EFI_CALL(efi_selftest(&image_obj->header, &systab));
> - bootefi_run_finish(image_obj, loaded_image_info);
> + efi_restore_gd();
> + free(loaded_image_info->load_options);
> + if (efi_delete_handle(&image_obj->header) != EFI_SUCCESS)
> + log_err("Failed to delete loaded image handle\n");
>
> return ret != EFI_SUCCESS;
> }
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index e530bcf15f76..43ccf49abec4 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -618,7 +618,7 @@ void efi_add_handle(efi_handle_t obj);
> /* Create handle */
> efi_status_t efi_create_handle(efi_handle_t *handle);
> /* Delete handle */
> -void efi_delete_handle(efi_handle_t obj);
> +efi_status_t efi_delete_handle(efi_handle_t obj);
> /* Call this to validate a handle and find the EFI object for it */
> struct efi_object *efi_search_obj(const efi_handle_t handle);
> /* Locate device_path handle */
> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c
> index 45f935198874..d8755541fc14 100644
> --- a/lib/efi_driver/efi_uclass.c
> +++ b/lib/efi_driver/efi_uclass.c
> @@ -285,10 +285,8 @@ static efi_status_t efi_add_driver(struct driver *drv)
> bp->ops = ops;
>
> ret = efi_create_handle(&bp->bp.driver_binding_handle);
> - if (ret != EFI_SUCCESS) {
> - free(bp);
> - goto out;
> - }
> + if (ret != EFI_SUCCESS)
> + goto err;
> bp->bp.image_handle = bp->bp.driver_binding_handle;
> ret = efi_add_protocol(bp->bp.driver_binding_handle,
> &efi_guid_driver_binding_protocol, bp);
> @@ -299,11 +297,12 @@ static efi_status_t efi_add_driver(struct driver *drv)
> if (ret != EFI_SUCCESS)
> goto err;
> }
> -out:
> - return ret;
>
> + return ret;
> err:
> - efi_delete_handle(bp->bp.driver_binding_handle);
> + if (bp->bp.driver_binding_handle &&
> + efi_delete_handle(bp->bp.driver_binding_handle) != EFI_SUCCESS)
> + log_err("Failed to delete handle\n");
You already log errors in efi_delete_handle(), please, avoid duplicate
log messages increasing code size (applies to several locations in this
patch).
Best regards
Heinrich
> free(bp);
> return ret;
> }
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index d75a3336e3f1..5123055d7f5d 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -59,6 +59,10 @@ static efi_handle_t current_image;
> static volatile gd_t *efi_gd, *app_gd;
> #endif
>
> +static efi_status_t efi_uninstall_protocol
> + (efi_handle_t handle, const efi_guid_t *protocol,
> + void *protocol_interface);
> +
> /* 1 if inside U-Boot code, 0 if inside EFI payload code */
> static int entry_count = 1;
> static int nesting_level;
> @@ -610,8 +614,8 @@ static efi_status_t efi_remove_all_protocols(const efi_handle_t handle)
> list_for_each_entry_safe(protocol, pos, &efiobj->protocols, link) {
> efi_status_t ret;
>
> - ret = efi_remove_protocol(handle, &protocol->guid,
> - protocol->protocol_interface);
> + ret = efi_uninstall_protocol(handle, &protocol->guid,
> + protocol->protocol_interface);
> if (ret != EFI_SUCCESS)
> return ret;
> }
> @@ -622,19 +626,23 @@ static efi_status_t efi_remove_all_protocols(const efi_handle_t handle)
> * efi_delete_handle() - delete handle
> *
> * @handle: handle to delete
> + *
> + * Return: status code
> */
> -void efi_delete_handle(efi_handle_t handle)
> +efi_status_t efi_delete_handle(efi_handle_t handle)
> {
> efi_status_t ret;
>
> ret = efi_remove_all_protocols(handle);
> - if (ret == EFI_INVALID_PARAMETER) {
> - log_err("Can't remove invalid handle %p\n", handle);
> - return;
> + if (ret != EFI_SUCCESS) {
> + log_err("Handle %p has protocols installed. Unable to delete\n", handle);
> + return ret;
> }
>
> list_del(&handle->link);
> free(handle);
> +
> + return ret;
> }
>
> /**
> @@ -1816,7 +1824,8 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> return ret;
> failure:
> printf("ERROR: Failure to install protocols for loaded image\n");
> - efi_delete_handle(&obj->header);
> + if (efi_delete_handle(&obj->header) != EFI_SUCCESS)
> + log_err("Failed to delete handle for loaded image\n");
> free(info);
> return ret;
> }
> @@ -2097,7 +2106,8 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy,
> info->parent_handle = parent_image;
> } else {
> /* The image is invalid. Release all associated resources. */
> - efi_delete_handle(*image_handle);
> + if (efi_delete_handle(*image_handle) != EFI_SUCCESS)
> + log_err("Failed to delete image handle\n");
> *image_handle = NULL;
> free(info);
> }
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 28c8cdf7100e..fae440a79225 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -537,7 +537,8 @@ static efi_status_t efi_disk_add_dev(
> }
> return EFI_SUCCESS;
> error:
> - efi_delete_handle(&diskobj->header);
> + if (efi_delete_handle(&diskobj->header) != EFI_SUCCESS)
> + log_err("Failed to delete handle\n");
> return ret;
> }
>
> @@ -577,7 +578,9 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle)
> }
> if (efi_link_dev(&disk->header, dev)) {
> efi_free_pool(disk->dp);
> - efi_delete_handle(&disk->header);
> + ret = efi_delete_handle(&disk->header);
> + if (ret != EFI_SUCCESS)
> + log_err("Failed to delete handle\n");
>
> return -EINVAL;
> }
> @@ -632,7 +635,9 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle)
> }
> if (efi_link_dev(&disk->header, dev)) {
> efi_free_pool(disk->dp);
> - efi_delete_handle(&disk->header);
> + ret = efi_delete_handle(&disk->header);
> + if (ret != EFI_SUCCESS)
> + log_err("Failed to delete handle\n");
>
> return -1;
> }
> @@ -708,6 +713,7 @@ int efi_disk_remove(void *ctx, struct event *event)
> efi_handle_t handle;
> struct blk_desc *desc;
> struct efi_disk_obj *diskobj = NULL;
> + efi_status_t ret;
>
> if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle))
> return 0;
> @@ -727,10 +733,13 @@ int efi_disk_remove(void *ctx, struct event *event)
> return 0;
> }
>
> + ret = efi_delete_handle(handle);
> + if (ret != EFI_SUCCESS)
> + return -1;
> +
> if (diskobj)
> efi_free_pool(diskobj->dp);
>
> - efi_delete_handle(handle);
> dev_tag_del(dev, DM_TAG_EFI);
>
> return 0;
> --
> 2.40.1
>
More information about the U-Boot
mailing list