[PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find
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; }
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
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));-- 2.37.2
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));-- 2.37.2
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);fmp = fmp_handler->protocol_interface;@@ -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));-- 2.37.2
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
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
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
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
participants (3)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Ilias Apalodimas