[PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[Heinrich Schuchardt]

On 10/11/22 13:08, Heinrich Schuchardt wrote:
> 
> 
> 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.

Since 755d42d4209e ("efi_loader: correct HandleProtocol()") we set agent 
handle = efi_root in the implementation of HandleProtocol(). So this 
part is ok.

Best regards

Heirnich

> 
>>
>>> 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
>>>>>