diff mbox series

[2/4,v2] efi_loader: check the status of disconnected drivers

Message ID 20230620061932.113292-3-ilias.apalodimas@linaro.org
State Accepted
Commit 747d16d93c3b03e835925dd9ecb8f8d0b8d1ad22
Headers show
Series Reconnect controllers on failues | expand

Commit Message

Ilias Apalodimas June 20, 2023, 6:19 a.m. UTC 
efi_uninstall_protocol() calls efi_disconnect_all_drivers() but never
checks the return value.  Instead it tries to identify protocols that
are still open after closing the ones that were opened with
EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL, EFI_OPEN_PROTOCOL_GET_PROTOCOL
and EFI_OPEN_PROTOCOL_TEST_PROTOCOL.

Instead of doing that,  check the return value early and exit if
disconnecting the drivers failed.  Also reconnect all the drivers of
a handle if protocols are still found on the handle after disconnecting
controllers and closing the remaining protocols.

While at it fix a memory leak and properly free the opened protocol
information when closing a protocol.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_boottime.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

--
2.40.1

Comments

Heinrich Schuchardt June 20, 2023, 6:57 a.m. UTC | #1 
Am 20. Juni 2023 08:19:29 MESZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>efi_uninstall_protocol() calls efi_disconnect_all_drivers() but never
>checks the return value.  Instead it tries to identify protocols that
>are still open after closing the ones that were opened with
>EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL, EFI_OPEN_PROTOCOL_GET_PROTOCOL
>and EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
>
>Instead of doing that,  check the return value early and exit if
>disconnecting the drivers failed.  Also reconnect all the drivers of
>a handle if protocols are still found on the handle after disconnecting
>controllers and closing the remaining protocols.

We talk about a single protocol but possibly multiple open prototocol information records.

Othrwise looks good.

Best regards

Heinrich

>
>While at it fix a memory leak and properly free the opened protocol
>information when closing a protocol.
>
>Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>---
> lib/efi_loader/efi_boottime.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>index d44562d8f4e0..bfcd913dfad9 100644
>--- a/lib/efi_loader/efi_boottime.c
>+++ b/lib/efi_loader/efi_boottime.c
>@@ -1374,17 +1374,23 @@ static efi_status_t efi_uninstall_protocol
> 	if (r != EFI_SUCCESS)
> 		goto out;
> 	/* Disconnect controllers */
>-	efi_disconnect_all_drivers(efiobj, protocol, NULL);
>+	r = efi_disconnect_all_drivers(efiobj, protocol, NULL);
>+	if (r != EFI_SUCCESS) {
>+		r = EFI_ACCESS_DENIED;
>+		goto out;
>+	}
> 	/* Close protocol */
> 	list_for_each_entry_safe(item, pos, &handler->open_infos, link) {
> 		if (item->info.attributes ==
> 			EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL ||
> 		    item->info.attributes == EFI_OPEN_PROTOCOL_GET_PROTOCOL ||
> 		    item->info.attributes == EFI_OPEN_PROTOCOL_TEST_PROTOCOL)
>-			list_del(&item->link);
>+			efi_delete_open_info(item);
> 	}
>+	/* if agents didn't close the protocols properly */
> 	if (!list_empty(&handler->open_infos)) {
> 		r =  EFI_ACCESS_DENIED;
>+		EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));
> 		goto out;
> 	}
> 	r = efi_remove_protocol(handle, protocol, protocol_interface);
>--
>2.40.1
>
Heinrich Schuchardt July 9, 2023, 9:10 a.m. UTC | #2 
On 6/20/23 08:19, Ilias Apalodimas wrote:
> efi_uninstall_protocol() calls efi_disconnect_all_drivers() but never
> checks the return value.  Instead it tries to identify protocols that
> are still open after closing the ones that were opened with
> EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL, EFI_OPEN_PROTOCOL_GET_PROTOCOL
> and EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
>
> Instead of doing that,  check the return value early and exit if
> disconnecting the drivers failed.  Also reconnect all the drivers of
> a handle if protocols are still found on the handle after disconnecting
> controllers and closing the remaining protocols.
>
> While at it fix a memory leak and properly free the opened protocol
> information when closing a protocol.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index d44562d8f4e0..bfcd913dfad9 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1374,17 +1374,23 @@  static efi_status_t efi_uninstall_protocol
 	if (r != EFI_SUCCESS)
 		goto out;
 	/* Disconnect controllers */
-	efi_disconnect_all_drivers(efiobj, protocol, NULL);
+	r = efi_disconnect_all_drivers(efiobj, protocol, NULL);
+	if (r != EFI_SUCCESS) {
+		r = EFI_ACCESS_DENIED;
+		goto out;
+	}
 	/* Close protocol */
 	list_for_each_entry_safe(item, pos, &handler->open_infos, link) {
 		if (item->info.attributes ==
 			EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL ||
 		    item->info.attributes == EFI_OPEN_PROTOCOL_GET_PROTOCOL ||
 		    item->info.attributes == EFI_OPEN_PROTOCOL_TEST_PROTOCOL)
-			list_del(&item->link);
+			efi_delete_open_info(item);
 	}
+	/* if agents didn't close the protocols properly */
 	if (!list_empty(&handler->open_infos)) {
 		r =  EFI_ACCESS_DENIED;
+		EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));
 		goto out;
 	}
 	r = efi_remove_protocol(handle, protocol, protocol_interface);