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

[Heinrich Schuchardt]

On 10/6/22 09:38, Ilias Apalodimas wrote:
> On Thu, Oct 06, 2022 at 04:26:37AM +0200, Heinrich Schuchardt wrote:
>> 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 at 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.
>
> Ok I'll fix it in v2

We only install the device path file_path on mem_handle. The loaded
image itself is referenced by handle. So the only thing you missed here
was checking the return value.

Best regards

Heinrich

>
>>
>> Best regards
>>
>> Heinrich
>>
>>>>    	efi_free_pool(file_path);
>>>>    	return ret;
>>>>    }
>>>> --
>>>> 2.34.1
>>>>
>>
>
> Thanks
> /Ilias