[U-Boot] [PATCH 0/3] efi_loader: avoid use after free

[U-Boot] [PATCH 0/3] efi_loader: avoid use after free

[Heinrich Schuchardt]

A use after free may occur if efi_load_image() fails.

Heinrich Schuchardt (3):
  efi_loader: return status from efi_setup_loaded_image()
  efi_loader: new function efi_delete_handle()
  efi_loader: error handling in efi_load_image()

 include/efi_loader.h          |  10 +-
 lib/efi_loader/efi_boottime.c | 228 ++++++++++++++++++++++--------------------
 2 files changed, 129 insertions(+), 109 deletions(-)

-- 
2.14.2

[U-Boot] [PATCH 1/3] efi_loader: return status from efi_setup_loaded_image()

[Heinrich Schuchardt]

efi_setup_loaded_image() should return an error code indicating if
an error has occurred.

An error occurs if a protocol cannot be installed.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          |  8 +++++---
 lib/efi_loader/efi_boottime.c | 11 +++++++----
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index c0caabddb1..78237f14ae 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -249,9 +249,11 @@ uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
 int efi_memory_init(void);
 /* Adds new or overrides configuration table entry to the system table */
 efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void *table);
-void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *obj,
-			    struct efi_device_path *device_path,
-			    struct efi_device_path *file_path);
+/* Sets up a loaded image */
+efi_status_t efi_setup_loaded_image(
+			struct efi_loaded_image *info, struct efi_object *obj,
+			struct efi_device_path *device_path,
+			struct efi_device_path *file_path);
 efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
 				      void **buffer);
 
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index a37fb25638..a9ba1ac394 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1170,10 +1170,12 @@ static efi_status_t EFIAPI efi_install_configuration_table_ext(efi_guid_t *guid,
  * @obj			internal object associated with the loaded image
  * @device_path		device path of the loaded image
  * @file_path		file path of the loaded image
+ * @return		status code
  */
-void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *obj,
-			    struct efi_device_path *device_path,
-			    struct efi_device_path *file_path)
+efi_status_t efi_setup_loaded_image(
+			struct efi_loaded_image *info, struct efi_object *obj,
+			struct efi_device_path *device_path,
+			struct efi_device_path *file_path)
 {
 	efi_status_t ret;
 
@@ -1213,9 +1215,10 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob
 	if (ret != EFI_SUCCESS)
 		goto failure;
 
-	return;
+	return ret;
 failure:
 	printf("ERROR: Failure to install protocols for loaded image\n");
+	return ret;
 }
 
 /*
-- 
2.14.2

[U-Boot] [PATCH 2/3] efi_loader: new function efi_delete_handle()

[Heinrich Schuchardt]

Provide a function to remove a handle from the object list
after removing all protocols.

To avoid forward declarations other functions have to move up
in the coding.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          |   2 +
 lib/efi_loader/efi_boottime.c | 186 +++++++++++++++++++++++-------------------
 2 files changed, 102 insertions(+), 86 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 78237f14ae..6185055e78 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -190,6 +190,8 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path);
 void efi_add_handle(struct efi_object *obj);
 /* Create handle */
 efi_status_t efi_create_handle(void **handle);
+/* Delete handle */
+void efi_delete_handle(struct efi_object *obj);
 /* Call this to validate a handle and find the EFI object for it */
 struct efi_object *efi_search_obj(const void *handle);
 /* Find a protocol on a handle */
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index a9ba1ac394..7c8f3134d1 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -359,6 +359,106 @@ efi_status_t efi_create_handle(void **handle)
 	return r;
 }
 
+/*
+ * Find a protocol on a handle.
+ *
+ * @handle		handle
+ * @protocol_guid	GUID of the protocol
+ * @handler		reference to the protocol
+ * @return		status code
+ */
+efi_status_t efi_search_protocol(const void *handle,
+				 const efi_guid_t *protocol_guid,
+				 struct efi_handler **handler)
+{
+	struct efi_object *efiobj;
+	struct list_head *lhandle;
+
+	if (!handle || !protocol_guid)
+		return EFI_INVALID_PARAMETER;
+	efiobj = efi_search_obj(handle);
+	if (!efiobj)
+		return EFI_INVALID_PARAMETER;
+	list_for_each(lhandle, &efiobj->protocols) {
+		struct efi_handler *protocol;
+
+		protocol = list_entry(lhandle, struct efi_handler, link);
+		if (!guidcmp(protocol->guid, protocol_guid)) {
+			if (handler)
+				*handler = protocol;
+			return EFI_SUCCESS;
+		}
+	}
+	return EFI_NOT_FOUND;
+}
+
+/*
+ * Delete protocol from a handle.
+ *
+ * @handle			handle from which the protocol shall be deleted
+ * @protocol			GUID of the protocol to be deleted
+ * @protocol_interface		interface of the protocol implementation
+ * @return			status code
+ */
+efi_status_t efi_remove_protocol(const void *handle, const efi_guid_t *protocol,
+				 void *protocol_interface)
+{
+	struct efi_handler *handler;
+	efi_status_t ret;
+
+	ret = efi_search_protocol(handle, protocol, &handler);
+	if (ret != EFI_SUCCESS)
+		return ret;
+	if (guidcmp(handler->guid, protocol))
+		return EFI_INVALID_PARAMETER;
+	list_del(&handler->link);
+	free(handler);
+	return EFI_SUCCESS;
+}
+
+/*
+ * Delete all protocols from a handle.
+ *
+ * @handle	handle from which the protocols shall be deleted
+ * @return	status code
+ */
+efi_status_t efi_remove_all_protocols(const void *handle)
+{
+	struct efi_object *efiobj;
+	struct list_head *lhandle;
+	struct list_head *pos;
+
+	efiobj = efi_search_obj(handle);
+	if (!efiobj)
+		return EFI_INVALID_PARAMETER;
+	list_for_each_safe(lhandle, pos, &efiobj->protocols) {
+		struct efi_handler *protocol;
+		efi_status_t ret;
+
+		protocol = list_entry(lhandle, struct efi_handler, link);
+
+		ret = efi_remove_protocol(handle, protocol->guid,
+					  protocol->protocol_interface);
+		if (ret != EFI_SUCCESS)
+			return ret;
+	}
+	return EFI_SUCCESS;
+}
+
+/*
+ * Delete handle.
+ *
+ * @handle	handle to delete
+ */
+void efi_delete_handle(struct efi_object *obj)
+{
+	if (!obj)
+		return;
+	efi_remove_all_protocols(obj->handle);
+	list_del(&obj->link);
+	free(obj);
+}
+
 /*
  * Our event capabilities are very limited. Only a small limited
  * number of events is allowed to coexist.
@@ -717,39 +817,6 @@ struct efi_object *efi_search_obj(const void *handle)
 	return NULL;
 }
 
-/*
- * Find a protocol on a handle.
- *
- * @handle		handle
- * @protocol_guid	GUID of the protocol
- * @handler		reference to the protocol
- * @return		status code
- */
-efi_status_t efi_search_protocol(const void *handle,
-				 const efi_guid_t *protocol_guid,
-				 struct efi_handler **handler)
-{
-	struct efi_object *efiobj;
-	struct list_head *lhandle;
-
-	if (!handle || !protocol_guid)
-		return EFI_INVALID_PARAMETER;
-	efiobj = efi_search_obj(handle);
-	if (!efiobj)
-		return EFI_INVALID_PARAMETER;
-	list_for_each(lhandle, &efiobj->protocols) {
-		struct efi_handler *protocol;
-
-		protocol = list_entry(lhandle, struct efi_handler, link);
-		if (!guidcmp(protocol->guid, protocol_guid)) {
-			if (handler)
-				*handler = protocol;
-			return EFI_SUCCESS;
-		}
-	}
-	return EFI_NOT_FOUND;
-}
-
 /*
  * Install new protocol on a handle.
  *
@@ -780,59 +847,6 @@ efi_status_t efi_add_protocol(const void *handle, const efi_guid_t *protocol,
 	return EFI_SUCCESS;
 }
 
-/*
- * Delete protocol from a handle.
- *
- * @handle			handle from which the protocol shall be deleted
- * @protocol			GUID of the protocol to be deleted
- * @protocol_interface		interface of the protocol implementation
- * @return			status code
- */
-efi_status_t efi_remove_protocol(const void *handle, const efi_guid_t *protocol,
-				 void *protocol_interface)
-{
-	struct efi_handler *handler;
-	efi_status_t ret;
-
-	ret = efi_search_protocol(handle, protocol, &handler);
-	if (ret != EFI_SUCCESS)
-		return ret;
-	if (guidcmp(handler->guid, protocol))
-		return EFI_INVALID_PARAMETER;
-	list_del(&handler->link);
-	free(handler);
-	return EFI_SUCCESS;
-}
-
-/*
- * Delete all protocols from a handle.
- *
- * @handle			handle from which the protocols shall be deleted
- * @return			status code
- */
-efi_status_t efi_remove_all_protocols(const void *handle)
-{
-	struct efi_object *efiobj;
-	struct list_head *lhandle;
-	struct list_head *pos;
-
-	efiobj = efi_search_obj(handle);
-	if (!efiobj)
-		return EFI_INVALID_PARAMETER;
-	list_for_each_safe(lhandle, pos, &efiobj->protocols) {
-		struct efi_handler *protocol;
-		efi_status_t ret;
-
-		protocol = list_entry(lhandle, struct efi_handler, link);
-
-		ret = efi_remove_protocol(handle, protocol->guid,
-					  protocol->protocol_interface);
-		if (ret != EFI_SUCCESS)
-			return ret;
-	}
-	return EFI_SUCCESS;
-}
-
 /*
  * Install protocol interface.
  *
-- 
2.14.2

[U-Boot] [PATCH 3/3] efi_loader: error handling in efi_load_image()

[Heinrich Schuchardt]

If a failure occurs when trying to load an image, it is insufficient
to free() the EFI object. We must remove it from the object list,
too. Otherwise a use after free will occur the next time we
iterate over the object list.

Furthermore errors in setting up the image should be handled.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 7c8f3134d1..b90bd0b426 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1308,6 +1308,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
 {
 	struct efi_loaded_image *info;
 	struct efi_object *obj;
+	efi_status_t ret;
 
 	EFI_ENTRY("%d, %p, %p, %p, %ld, %p", boot_policy, parent_image,
 		  file_path, source_buffer, source_size, image_handle);
@@ -1317,41 +1318,39 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
 
 	if (!source_buffer) {
 		struct efi_device_path *dp, *fp;
-		efi_status_t ret;
 
 		ret = efi_load_image_from_path(file_path, &source_buffer);
-		if (ret != EFI_SUCCESS) {
-			free(info);
-			free(obj);
-			return EFI_EXIT(ret);
-		}
-
+		if (ret != EFI_SUCCESS)
+			goto failure;
 		/*
 		 * split file_path which contains both the device and
 		 * file parts:
 		 */
 		efi_dp_split_file_path(file_path, &dp, &fp);
-
-		efi_setup_loaded_image(info, obj, dp, fp);
+		ret = efi_setup_loaded_image(info, obj, dp, fp);
+		if (ret != EFI_SUCCESS)
+			goto failure;
 	} else {
 		/* In this case, file_path is the "device" path, ie.
 		 * something like a HARDWARE_DEVICE:MEMORY_MAPPED
 		 */
-		efi_setup_loaded_image(info, obj, file_path, NULL);
+		ret = efi_setup_loaded_image(info, obj, file_path, NULL);
+		if (ret != EFI_SUCCESS)
+			goto failure;
 	}
-
 	info->reserved = efi_load_pe(source_buffer, info);
 	if (!info->reserved) {
-		free(info);
-		free(obj);
-		return EFI_EXIT(EFI_UNSUPPORTED);
+		ret = EFI_UNSUPPORTED;
+		goto failure;
 	}
-
 	info->system_table = &systab;
 	info->parent_handle = parent_image;
 	*image_handle = obj->handle;
-
 	return EFI_EXIT(EFI_SUCCESS);
+failure:
+	free(info);
+	efi_delete_handle(obj);
+	return EFI_EXIT(ret);
 }
 
 /*
-- 
2.14.2