[PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[Heinrich Schuchardt]

On 10/11/22 09:35, AKASHI Takahiro wrote:
> On Tue, Oct 11, 2022 at 07:58:11AM +0200, Heinrich Schuchardt wrote:
>>
>>
>> On 10/11/22 02:49, AKASHI Takahiro wrote:
>>> The commit message is not accurate.
>>>
>>> On Fri, Oct 07, 2022 at 04:06:23PM +0200, Heinrich Schuchardt wrote:
>>>> The CloseProtocol() boot service requires a handle as first argument.
>>>> Passing the protocol interface is incorrect.
>>>
>>> Correct, but
>>>
>>>> CloseProtocol() only has an effect if called with a non-zero value for
>>>> agent_handle. HandleProtocol() uses an opaque agent_handle when invoking
>>>> OpenProtocol() (currently NULL).
>>>
>>> No. OpenProtocol() is called with efi_root as an agent handle.
>>> So, calling CloseProtocol() is a right thing at the end.
>>
>> Typically an agent handle is used to relate to a driver exposing the driver
>> binding protocol.
> 
> Why can't we, other than a driver, call HandleProtocol()
> as a convenient way of accessing an interface?

The description of HandleProtocol() clearly says that it is deprecated.

The assumption that the UEFI specification makes in it is example code 
that you never be able to close a protocol opened with HandleProtocol.

After the first usage of handle protocol the open protocol information 
with the opaque agent handle will block the protocol interface from ever 
being removed by the driver exposing it.

> 
>> The root node does not expose the driver binding protocol.
> 
> So do you mean the current implementation of HandleProtocol() is wrong?

Yes. If you ever install a boot time driver, it might remove a protocol 
interface which is actually still in use.

> 
>> Why would you want to create an open protocol information entry here?
> 
> To access get_image_info() quickly.

This is not related to an open protocol information (see the UEFI spec 
description of OpenProtocolInformation()).

Best regards

Heinrich

> 
>> Do you think anything with the code after the patch is wrong?
> 
> No reason to replace handle_protocol().
> 
> Another example is here:
> efi_load_image_from_path()
>      efi_handle_protocol(device, guid, (void **)&load_file_protocol));
>      ...
>      efi_close_protocol(device, guid, efi_root, NULL);
> 
> I believe that this function is anything but a driver.
> I think using HandleProtocol() (or preferably OpenProtocol()) and CloseProtocol()
> in pair seems totally sane.
> 
> -Takahiro Akashi
> 
> 
> 
>> Best regards
>>
>> Heinrich
>>
>>>
>>>> Therefore HandleProtocol() should be
>>>> avoided.
>>>>
>>>> * Replace the LocateHandle() call by efi_search_protocol().
>>>
>>> LocateHandle() -> efi_handle_protocol()
>>>
>>> So you could have fixed this way:
>>>       EFI_CALL(efi_close_protocol(handle, ..., &efi_root, NULL);
>>>
>>> I preferred to use EFI_CALL() over this file as you can see.
>>>
>>> -Takahiro Akashi
>>>
>>>> * Remove the CloseProtocol() call.
>>>>
>>>> Fixes: 8d99026f0697 ("efi_loader: capsule: support firmware update")
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>>> ---
>>>>    lib/efi_loader/efi_capsule.c | 14 ++++++--------
>>>>    1 file changed, 6 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
>>>> index b6bd2d6af8..397e393a18 100644
>>>> --- a/lib/efi_loader/efi_capsule.c
>>>> +++ b/lib/efi_loader/efi_capsule.c
>>>> @@ -159,12 +159,14 @@ efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
>>>>    	efi_status_t ret;
>>>>    	for (i = 0, handle = handles; i < no_handles; i++, handle++) {
>>>> -		ret = EFI_CALL(efi_handle_protocol(
>>>> -				*handle,
>>>> -				&efi_guid_firmware_management_protocol,
>>>> -				(void **)&fmp));
>>>> +		struct efi_handler *fmp_handler;
>>>> +
>>>> +		ret = efi_search_protocol(
>>>> +				*handle, &efi_guid_firmware_management_protocol,
>>>> +				&fmp_handler);
>>>>    		if (ret != EFI_SUCCESS)
>>>>    			continue;
>>>> +		fmp = fmp_handler->protocol_interface;
>>>>    		/* get device's image info */
>>>>    		info_size = 0;
>>>> @@ -215,10 +217,6 @@ efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
>>>>    skip:
>>>>    		efi_free_pool(package_version_name);
>>>>    		free(image_info);
>>>> -		EFI_CALL(efi_close_protocol(
>>>> -				(efi_handle_t)fmp,
>>>> -				&efi_guid_firmware_management_protocol,
>>>> -				NULL, NULL));
>>>>    		if (found)
>>>>    			return fmp;
>>>>    	}
>>>> -- 
>>>> 2.37.2
>>>>