Re: [PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple

On 10/6/22 15:08, Ilias Apalodimas wrote:

A following patch is cleaning up the core EFI code trying to remove sequences of efi_create_handle, efi_add_protocol.

Although this works fine there's a problem with the latter since it is usually combined with efi_delete_handle() which blindly removes all protocols on a handle and deletes the handle. We should try to adhere to the EFI spec which only deletes a handle if the last instance of a protocol has been removed. Another problem is that efi_delete_handle() never checks for opened protocols, but the EFI spec defines that the caller is responsible for ensuring that there are no references to a protocol interface that is going to be removed.

So let's fix this by replacing all callsites of efi_create_handle(), efi_add_protocol() , efi_delete_handle() with Install/UninstallMultipleProtocol.

In order to do that redefine functions that can be used by the U-Boot proper internally and add '_ext' variants that will be used from the EFI API

Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

include/efi.h | 2 + include/efi_loader.h | 6 +- lib/efi_loader/efi_boottime.c | 214 +++++++++++++++++++++++-------- lib/efi_loader/efi_capsule.c | 15 ++- lib/efi_loader/efi_console.c | 14 +- lib/efi_loader/efi_disk.c | 10 +- lib/efi_loader/efi_load_initrd.c | 15 +-- lib/efi_loader/efi_root_node.c | 44 +++---- 8 files changed, 216 insertions(+), 104 deletions(-)

diff --git a/include/efi.h b/include/efi.h index 6159f34ad2be..42f4e58a917e 100644 --- a/include/efi.h +++ b/include/efi.h @@ -37,12 +37,14 @@ #define EFIAPI __attribute__((ms_abi)) #define efi_va_list __builtin_ms_va_list #define efi_va_start __builtin_ms_va_start +#define efi_va_copy __builtin_ms_va_copy #define efi_va_arg __builtin_va_arg #define efi_va_end __builtin_ms_va_end #else #define EFIAPI asmlinkage #define efi_va_list va_list #define efi_va_start va_start +#define efi_va_copy va_copy #define efi_va_arg va_arg #define efi_va_end va_end #endif /* __x86_64__ */ diff --git a/include/efi_loader.h b/include/efi_loader.h index ad01395b39c3..2b294d64efd0 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -655,8 +655,10 @@ efi_status_t efi_remove_protocol(const efi_handle_t handle, /* Delete all protocols from a handle */ efi_status_t efi_remove_all_protocols(const efi_handle_t handle); /* Install multiple protocol interfaces */ -efi_status_t EFIAPI efi_install_multiple_protocol_interfaces

• 		(efi_handle_t *handle, ...);
  

+efi_status_t EFIAPI +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...); +efi_status_t EFIAPI +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...); /* Get handles that support a given protocol */ efi_status_t EFIAPI efi_locate_handle_buffer( enum efi_locate_search_type search_type, diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 1bfd094e89f8..e776d25830f0 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -2590,35 +2590,31 @@ found: }

/**

• • efi_install_multiple_protocol_interfaces() - Install multiple protocol

• • efi_install_multiple_protocol_interfaces_int() - Install multiple protocol

  •                                          interfaces
    

  • @handle: handle on which the protocol interfaces shall be installed

• • @...: NULL terminated argument list with pairs of protocol GUIDS and

• •      interfaces
    

• • @argptr: va_list of args

• • This function implements the MultipleProtocolInterfaces service.
• • See the Unified Extensible Firmware Interface (UEFI) specification for
• • details.

• • Core functionality of efi_install_multiple_protocol_interfaces
• • Must not be called directly
  • Return: status code
  */

-efi_status_t EFIAPI efi_install_multiple_protocol_interfaces

• 		(efi_handle_t *handle, ...)
  

+static efi_status_t EFIAPI +efi_install_multiple_protocol_interfaces_int(efi_handle_t *handle,

• 			     efi_va_list argptr)
  

  {

• EFI_ENTRY("%p", handle);
• efi_va_list argptr; const efi_guid_t *protocol; void *protocol_interface; efi_handle_t old_handle;
• efi_status_t r = EFI_SUCCESS;

• efi_status_t ret = EFI_SUCCESS; int i = 0;

• efi_va_list argptr_copy;

  if (!handle)

• return EFI_EXIT(EFI_INVALID_PARAMETER);
  

• return EFI_INVALID_PARAMETER;
  
  

• efi_va_start(argptr, handle);

• efi_va_copy(argptr_copy, argptr); for (;;) { protocol = efi_va_arg(argptr, efi_guid_t*); if (!protocol)

@@ -2628,104 +2624,212 @@ efi_status_t EFIAPI efi_install_multiple_protocol_interfaces if (!guidcmp(protocol, &efi_guid_device_path)) { struct efi_device_path *dp = protocol_interface;

• 	r = EFI_CALL(efi_locate_device_path(protocol, &dp,
  

• 					    &old_handle));
  

• 	if (r == EFI_SUCCESS &&
  

• 	ret = EFI_CALL(efi_locate_device_path(protocol, &dp,
  

• 					      &old_handle));
  

• 	if (ret == EFI_SUCCESS &&
      dp->type == DEVICE_PATH_TYPE_END) {
  	EFI_PRINT("Path %pD already installed\n",
  		  protocol_interface);
  

• 		r = EFI_ALREADY_STARTED;
  

• 		ret = EFI_ALREADY_STARTED;
  	break;
  }
  

  }

• r = EFI_CALL(efi_install_protocol_interface(
  

• 				handle, protocol,
  

• 				EFI_NATIVE_INTERFACE,
  

• 				protocol_interface));
  

• if (r != EFI_SUCCESS)
  

• ret = EFI_CALL(efi_install_protocol_interface(handle, protocol,
  

• 					      EFI_NATIVE_INTERFACE,
  

• 					      protocol_interface));
  

• if (ret != EFI_SUCCESS)
  break;
  

  i++; }

• efi_va_end(argptr);
• if (r == EFI_SUCCESS)

• return EFI_EXIT(r);
  

• if (ret == EFI_SUCCESS)

• goto out;
  
  

  /* If an error occurred undo all changes. */

• efi_va_start(argptr, handle); for (; i; --i) {

• protocol = efi_va_arg(argptr, efi_guid_t*);
  

• protocol_interface = efi_va_arg(argptr, void*);
  

• protocol = efi_va_arg(argptr_copy, efi_guid_t*);
  

• protocol_interface = efi_va_arg(argptr_copy, void*);
  

  EFI_CALL(efi_uninstall_protocol_interface(*handle, protocol, protocol_interface)); }

• efi_va_end(argptr);

• return EFI_EXIT(r);

+out:

• efi_va_end(argptr_copy);

• return ret;

• }

  /**

• • efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol

• •                                            interfaces
    

• • @handle: handle from which the protocol interfaces shall be removed

• • efi_install_multiple_protocol_interfaces() - Install multiple protocol

• •                                          interfaces
    

• • @handle: handle on which the protocol interfaces shall be installed
  • @...: NULL terminated argument list with pairs of protocol GUIDS and

  •      interfaces
    

• • This function implements the UninstallMultipleProtocolInterfaces service.

• • This is the function for internal usage in U-Boot. For the API function
• • implementing the InstallMultipleProtocol service see
• • efi_install_multiple_protocol_interfaces_ext()
• • Return: status code
• */

+efi_status_t EFIAPI +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...) +{

• efi_status_t ret;
• efi_va_list argptr;
• efi_va_start(argptr, handle);
• ret = efi_install_multiple_protocol_interfaces_int(handle, argptr);
• efi_va_end(argptr);
• return ret;

+}

+/**

• • efi_install_multiple_protocol_interfaces_ext() - Install multiple protocol

• •                                              interfaces
    

• • @handle: handle on which the protocol interfaces shall be installed
• • @...: NULL terminated argument list with pairs of protocol GUIDS and

• •      interfaces
    

• • This function implements the MultipleProtocolInterfaces service.
  • See the Unified Extensible Firmware Interface (UEFI) specification for
  • details.
  • Return: status code
  */

-static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(

• 	efi_handle_t handle, ...)
  

+static efi_status_t EFIAPI +efi_install_multiple_protocol_interfaces_ext(efi_handle_t *handle, ...) { EFI_ENTRY("%p", handle);

• efi_status_t ret; efi_va_list argptr;
• efi_va_start(argptr, handle);
• ret = efi_install_multiple_protocol_interfaces_int(handle, argptr);
• efi_va_end(argptr);
• return EFI_EXIT(ret);

+}

+/**

• • efi_uninstall_multiple_protocol_interfaces_int() - wrapper for uninstall

• •                                              multiple protocol
    

• •                                              interfaces
    

• • @handle: handle from which the protocol interfaces shall be removed
• • @argptr: va_list of args
• • Core functionality of efi_uninstall_multiple_protocol_interfaces
• • Must not be called directly
• • Return: status code
• */

+static efi_status_t EFIAPI +efi_uninstall_multiple_protocol_interfaces_int(efi_handle_t handle,

• 			       efi_va_list argptr)
  

+{ const efi_guid_t *protocol; void *protocol_interface;

• efi_status_t r = EFI_SUCCESS;

• efi_status_t ret; size_t i = 0;

• efi_va_list argptr_copy;

  if (!handle)

• return EFI_EXIT(EFI_INVALID_PARAMETER);
  

• return EFI_INVALID_PARAMETER;
  
  

• efi_va_start(argptr, handle);

• efi_va_copy(argptr_copy, argptr); for (;;) { protocol = efi_va_arg(argptr, efi_guid_t*); if (!protocol) break; protocol_interface = efi_va_arg(argptr, void*);

• r = efi_uninstall_protocol(handle, protocol,
  

• 			   protocol_interface);
  

• if (r != EFI_SUCCESS)
  

• ret = efi_uninstall_protocol(handle, protocol,
  

• 			     protocol_interface);
  

• if (ret != EFI_SUCCESS)
  break;
  

  i++; }

• efi_va_end(argptr);
• if (r == EFI_SUCCESS) {

• if (ret == EFI_SUCCESS) { /* If the last protocol has been removed, delete the handle. */ if (list_empty(&handle->protocols)) { list_del(&handle->link); free(handle); }

• return EFI_EXIT(r);
  

• goto out;
  

  }

  /* If an error occurred undo all changes. */

• efi_va_start(argptr, handle); for (; i; --i) {

• protocol = efi_va_arg(argptr, efi_guid_t*);
  

• protocol_interface = efi_va_arg(argptr, void*);
  

• protocol = efi_va_arg(argptr_copy, efi_guid_t*);
  

• protocol_interface = efi_va_arg(argptr_copy, void*);
  

  EFI_CALL(efi_install_protocol_interface(&handle, protocol, EFI_NATIVE_INTERFACE, protocol_interface)); }
• /*

• * If any errors are generated while the protocol interfaces are being
  

• * uninstalled, then the protocols uninstalled prior to the error will
  

• * be reinstalled using InstallProtocolInterface() and the status code
  

• * EFI_INVALID_PARAMETER is returned.
  

• */
  

• ret = EFI_INVALID_PARAMETER;

+out:

• efi_va_end(argptr_copy);
• return ret;

+}

+/**

• • efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol

• •                                            interfaces
    

• • @handle: handle from which the protocol interfaces shall be removed
• • @...: NULL terminated argument list with pairs of protocol GUIDS and

• •      interfaces
    

• • This function implements the UninstallMultipleProtocolInterfaces service.
• • This is the function for internal usage in U-Boot. For the API function
• • implementing the UninstallMultipleProtocolInterfaces service see
• • efi_uninstall_multiple_protocol_interfaces_ext()
• • Return: status code
• */

+efi_status_t EFIAPI +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...) +{

• efi_status_t ret;
• efi_va_list argptr;
• efi_va_start(argptr, handle);
• ret = efi_uninstall_multiple_protocol_interfaces_int(handle, argptr); efi_va_end(argptr);
• return ret;

+}

+/**

• • efi_uninstall_multiple_protocol_interfaces_ext() - uninstall multiple protocol

• •                                                interfaces
    

• • @handle: handle from which the protocol interfaces shall be removed
• • @...: NULL terminated argument list with pairs of protocol GUIDS and

• •      interfaces
    

• • This function implements the UninstallMultipleProtocolInterfaces service.
• • See the Unified Extensible Firmware Interface (UEFI) specification for
• • details.
• • Return: status code
• */

+static efi_status_t EFIAPI +efi_uninstall_multiple_protocol_interfaces_ext(efi_handle_t handle, ...) +{

• EFI_ENTRY("%p", handle);
• efi_status_t ret;
• efi_va_list argptr;

• /* In case of an error always return EFI_INVALID_PARAMETER */
• return EFI_EXIT(EFI_INVALID_PARAMETER);

• efi_va_start(argptr, handle);

• ret = efi_uninstall_multiple_protocol_interfaces_int(handle, argptr);

• efi_va_end(argptr);

• return EFI_EXIT(ret); }

  /**

@@ -3785,9 +3889,9 @@ static struct efi_boot_services efi_boot_services = { .locate_handle_buffer = efi_locate_handle_buffer, .locate_protocol = efi_locate_protocol, .install_multiple_protocol_interfaces =

• 	efi_install_multiple_protocol_interfaces,
  

• 	efi_install_multiple_protocol_interfaces_ext,
  

  .uninstall_multiple_protocol_interfaces =

• 	efi_uninstall_multiple_protocol_interfaces,
  

• 	efi_uninstall_multiple_protocol_interfaces_ext,
  

  .calculate_crc32 = efi_calculate_crc32, .copy_mem = efi_copy_mem, .set_mem = efi_set_mem,

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index a6b98f066a0b..b6bd2d6af882 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -636,17 +636,18 @@ efi_status_t __weak efi_load_capsule_drivers(void)

if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) { handle = NULL;

• ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
  

• 		&handle, &efi_guid_firmware_management_protocol,
  

• 		&efi_fmp_fit, NULL));
  

• ret = efi_install_multiple_protocol_interfaces(&handle,
  

• 					       &efi_guid_firmware_management_protocol,
  

• 					       &efi_fmp_fit,
  

• 					       NULL);
  

  }

  if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) { handle = NULL;

• ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
  

• 		&handle,
  

• 		&efi_guid_firmware_management_protocol,
  

• 		&efi_fmp_raw, NULL));
  

• ret = efi_install_multiple_protocol_interfaces(&handle,
  

• 					       &efi_guid_firmware_management_protocol,
  

• 					       &efi_fmp_raw,
  

• 					       NULL);
  

  }

  return ret;

diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index cf9fbd9cb54d..3354b217a9a4 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -1278,12 +1278,14 @@ efi_status_t efi_console_register(void) struct efi_device_path *dp;

/* Install protocols on root node */

• r = EFI_CALL(efi_install_multiple_protocol_interfaces

•      (&efi_root,
  

•       &efi_guid_text_output_protocol, &efi_con_out,
  

•       &efi_guid_text_input_protocol, &efi_con_in,
  

•       &efi_guid_text_input_ex_protocol, &efi_con_in_ex,
  

•       NULL));
  

• r = efi_install_multiple_protocol_interfaces(&efi_root,

• 				     &efi_guid_text_output_protocol,
  

• 				     &efi_con_out,
  

• 				     &efi_guid_text_input_protocol,
  

• 				     &efi_con_in,
  

• 				     &efi_guid_text_input_ex_protocol,
  

• 				     &efi_con_in_ex,
  

• 				     NULL);
  
  

  /* Create console node and install device path protocols */ if (CONFIG_IS_ENABLED(DM_SERIAL)) {

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 39ea1a68a683..fb96b0508528 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -449,10 +449,12 @@ static efi_status_t efi_disk_add_dev( * in this case. */ handle = &diskobj->header;

• ret = EFI_CALL(efi_install_multiple_protocol_interfaces(

• 	&handle, &efi_guid_device_path, diskobj->dp,
  

• 	&efi_block_io_guid, &diskobj->ops,
  

• 	guid, NULL, NULL));
  

• ret = efi_install_multiple_protocol_interfaces(&handle,

• 				       &efi_guid_device_path,
  

• 				       diskobj->dp,
  

• 				       &efi_block_io_guid,
  

• 				       &diskobj->ops, guid,
  

• 				       NULL, NULL);
  

  if (ret != EFI_SUCCESS) goto error;

diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c index 3d6044f76047..87fde3f88c2b 100644 --- a/lib/efi_loader/efi_load_initrd.c +++ b/lib/efi_loader/efi_load_initrd.c @@ -208,14 +208,13 @@ efi_status_t efi_initrd_register(void) if (ret != EFI_SUCCESS) return ret;

• ret = EFI_CALL(efi_install_multiple_protocol_interfaces

•        (&efi_initrd_handle,
  

• 	/* initramfs */
  

• 	&efi_guid_device_path, &dp_lf2_handle,
  

• 	/* LOAD_FILE2 */
  

• 	&efi_guid_load_file2_protocol,
  

• 	(void *)&efi_lf2_protocol,
  

• 	NULL));
  

• ret = efi_install_multiple_protocol_interfaces(&efi_initrd_handle,

• 				       /* initramfs */
  

• 				       &efi_guid_device_path, &dp_lf2_handle,
  

• 				       /* LOAD_FILE2 */
  

• 				       &efi_guid_load_file2_protocol,
  

• 				       (void *)&efi_lf2_protocol,
  

• 				       NULL);
  
  

  return ret; }

diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c index 739c6867f412..b4696d54c33d 100644 --- a/lib/efi_loader/efi_root_node.c +++ b/lib/efi_loader/efi_root_node.c @@ -49,38 +49,38 @@ efi_status_t efi_root_node_register(void) dp->end.length = sizeof(struct efi_device_path);

/* Create root node and install protocols */

• ret = EFI_CALL(efi_install_multiple_protocol_interfaces

• 	(&efi_root,
  

• 	 /* Device path protocol */
  

• 	 &efi_guid_device_path, dp,
  

• ret = efi_install_multiple_protocol_interfaces

• (&efi_root,
  

•  /* Device path protocol */
  

•  &efi_guid_device_path, dp,
  

  #if CONFIG_IS_ENABLED(EFI_DEVICE_PATH_TO_TEXT)

• 	 /* Device path to text protocol */
  

• 	 &efi_guid_device_path_to_text_protocol,
  

• 	 (void *)&efi_device_path_to_text,
  

•  /* Device path to text protocol */
  

•  &efi_guid_device_path_to_text_protocol,
  

•  (void *)&efi_device_path_to_text,
  

  #endif #ifdef CONFIG_EFI_DEVICE_PATH_UTIL

• 	 /* Device path utilities protocol */
  

• 	 &efi_guid_device_path_utilities_protocol,
  

• 	 (void *)&efi_device_path_utilities,
  

•  /* Device path utilities protocol */
  

•  &efi_guid_device_path_utilities_protocol,
  

•  (void *)&efi_device_path_utilities,
  

  #endif #ifdef CONFIG_EFI_DT_FIXUP

• 	 /* Device-tree fix-up protocol */
  

• 	 &efi_guid_dt_fixup_protocol,
  

• 	 (void *)&efi_dt_fixup_prot,
  

•  /* Device-tree fix-up protocol */
  

•  &efi_guid_dt_fixup_protocol,
  

•  (void *)&efi_dt_fixup_prot,
  

  #endif #if CONFIG_IS_ENABLED(EFI_UNICODE_COLLATION_PROTOCOL2)

• 	 &efi_guid_unicode_collation_protocol2,
  

• 	 (void *)&efi_unicode_collation_protocol2,
  

•  &efi_guid_unicode_collation_protocol2,
  

•  (void *)&efi_unicode_collation_protocol2,
  

  #endif #if CONFIG_IS_ENABLED(EFI_LOADER_HII)

• 	 /* HII string protocol */
  

• 	 &efi_guid_hii_string_protocol,
  

• 	 (void *)&efi_hii_string,
  

• 	 /* HII database protocol */
  

• 	 &efi_guid_hii_database_protocol,
  

• 	 (void *)&efi_hii_database,
  

•  /* HII string protocol */
  

•  &efi_guid_hii_string_protocol,
  

•  (void *)&efi_hii_string,
  

•  /* HII database protocol */
  

•  &efi_guid_hii_database_protocol,
  

•  (void *)&efi_hii_database,
  

All these (void *) conversions seem superfluous as the expected parameter is a (void *) and accepts any pointer.

Otherwise

Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de

#endif

• 	 NULL));
  

•  NULL);
  

  efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE; return ret; }