[PATCH v2] efi_loader: make efi_delete_handle() follow the EFI spec

[Heinrich Schuchardt]

On 7/24/23 12:17, 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>
> ---
> Changes since v1:
> - only report errors internally in efi_delete_handle()
>
>   cmd/bootefi.c                 | 21 ++++++---------------
>   include/efi_loader.h          |  2 +-
>   lib/efi_driver/efi_uclass.c   | 12 +++++-------
>   lib/efi_loader/efi_boottime.c | 20 ++++++++++++++------
>   lib/efi_loader/efi_disk.c     |  6 +++++-
>   5 files changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 5c0afec1544d..f73d6eb0e2d8 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,12 @@ 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 (ret != EFI_SUCCESS)
> +		efi_delete_handle(&image_obj->header);
> +	else
> +		ret = efi_delete_handle(&image_obj->header);
>
>   	return ret != EFI_SUCCESS;
>   }
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index b5fa0fe01ded..3a64eb9c6672 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -629,7 +629,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..66a45e156d60 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,11 @@ 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);
>   	free(bp);
>   	return ret;
>   }
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 052fe481e473..0e89c8505b11 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;
>   }
>
>   /**
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 28c8cdf7100e..8983b86886b3 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -708,6 +708,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 +728,13 @@ int efi_disk_remove(void *ctx, struct event *event)
>   		return 0;
>   	}
>
> +	ret = efi_delete_handle(handle);
> +	if (ret != EFI_SUCCESS)
> +		return -1;

I guess we should add a comment here explaining that the DM device
should not be removed if there still are open protocols for the handle.

Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

> +
>   	if (diskobj)
>   		efi_free_pool(diskobj->dp);
>
> -	efi_delete_handle(handle);
>   	dev_tag_del(dev, DM_TAG_EFI);
>
>   	return 0;
> --
> 2.40.1
>