Re: [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 10/6/22 03:53, AKASHI Takahiro wrote:
> On Wed, Oct 05, 2022 at 06:26:02PM +0300, Ilias Apalodimas wrote:
>> In general handles should only be deleted if the last remaining protocol
>> is removed.  Instead of explicitly calling
>> efi_create_handle -> efi_add_protocol -> efi_delete_handle which blindly
>> removes all protocols from a handle before removing it,  use
>> InstallMultiple/UninstallMultiple which adheres to the EFI spec and only
>> deletes a handle if there are no additional protocols present
>>
>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> ---
>>   cmd/bootefi.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 3041873afbbc..4ab68868cc7e 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -509,12 +509,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
>>   		 * Make sure that device for device_path exist
>>   		 * in load_image(). Otherwise, shell and grub will fail.
>>   		 */
>> -		ret = efi_create_handle(&mem_handle);
>> -		if (ret != EFI_SUCCESS)
>> -			goto out;
>> -
>> -		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>> -				       file_path);
>> +		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
>> +							       &efi_guid_device_path,
>> +							       file_path, NULL);
>
> nitpick: NULL -> NULL, NULL
> UEFI spec seems to require "A variable argument list containing pairs of
> protocol GUIDs and protocol interfaces" even if a protocol interface won't be
> used with GUID as NULL.

The spec also has:
"The pairs of arguments are removed in order from the variable argument
list until a NULL protocol GUID value is found."

This is what a calls inside EDK II looks like:

   Status = CoreInstallMultipleProtocolInterfaces (
              &mDecompressHandle,
              &gEfiDecompressProtocolGuid,
              &gEfiDecompress,
              NULL
              );

       gBS->UninstallMultipleProtocolInterfaces (
              Controller,
              &gEfiCallerIdGuid,
              AtaBusDriverData,
              NULL
              );

So I am happy with a single NULL here.

>
> -Takahiro Akashi
>
>>   		if (ret != EFI_SUCCESS)
>>   			goto out;
>>   		msg_path = file_path;
>> @@ -542,7 +539,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
>>   	ret = do_bootefi_exec(handle, load_options);
>>
>>   out:
>> -	efi_delete_handle(mem_handle);
>> +	efi_uninstall_multiple_protocol_interfaces(mem_handle,
>> +						   &efi_guid_device_path,
>> +						   file_path, NULL);

We have installed a lot more protocols. The binary may have installed
additional protocols. Consider the case of a boottime driver. To delete
the handle we would have to remove all installed protocols.

UninstallMultipleProtocolInterfaces() may fail if one of the protocols
was opened with ByDriver or ByChildcontroller. The return value has to
be considered before freeing file_path.

Best regards

Heinrich

>>   	efi_free_pool(file_path);
>>   	return ret;
>>   }
>> --
>> 2.34.1
>>