[PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[Heinrich Schuchardt]

The CloseProtocol() boot service requires a handle as first argument.
Passing the protocol interface is incorrect.

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). Therefore HandleProtocol() should be
avoided.

* Replace the LocateHandle() call by efi_search_protocol().
* Remove the CloseProtocol() call.

Fixes: 8d99026f0697 ("efi_loader: capsule: support firmware update")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@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

Re: [PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[Ilias Apalodimas]

Hi Heinrich

On Fri, 7 Oct 2022 at 17:06, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> The CloseProtocol() boot service requires a handle as first argument.
> Passing the protocol interface is incorrect.
>
> 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). Therefore HandleProtocol() should be
> avoided.

FWIW HandleProtocol is also obsolete so we should probably just leave
it for < 1.10 EFI apps and replace all calls with OpenProtocol (or
efi_search_protocol()) in the future

>
> * Replace the LocateHandle() call by efi_search_protocol().
> * Remove the CloseProtocol() call.
>
> Fixes: 8d99026f0697 ("efi_loader: capsule: support firmware update")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@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
>

With or without the nits on the commit message
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[AKASHI Takahiro]

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.

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

Re: [PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[Heinrich Schuchardt]

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. The root node does not expose the driver 
binding protocol.

Why would you want to create an open protocol information entry here?

Do you think anything with the code after the patch is wrong?

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

Re: [PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[AKASHI Takahiro]

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 root node does not expose the driver binding protocol.

So do you mean the current implementation of HandleProtocol() is wrong?

> Why would you want to create an open protocol information entry here?

To access get_image_info() quickly.

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

Re: [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@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
>>>>

Re: [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@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
>>>>>

Re: [PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[AKASHI Takahiro]

On Tue, Oct 11, 2022 at 01:08:26PM +0200, 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.

I don't understand this statement.
I never find any description of "you never be able to close a protocol opened
with HandleProtocol" in the spec.

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

Again, what about efi_load_image_from_path() that I gave in another example?
I think that you have something to do *before* you submit this patch.

Anyhow, your commit message is not accurate.

-Takahiro Akashi


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

Re: [PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[AKASHI Takahiro]

On Tue, Oct 11, 2022 at 01:12:18PM +0200, Heinrich Schuchardt wrote:
> 
> 
> 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.

That is why I said using HandleProtocl() is valid and that
your commit message is not accurate.

-Takahiro Akashi


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