[U-Boot] [PATCH v2 0/2] efi_loader: parameter checks in StartImage and Exit()

[U-Boot] [PATCH v2 0/2] efi_loader: parameter checks in StartImage and Exit()

[Heinrich Schuchardt]

Add parameter checks in the StartImage() and Exit() boottime services:
- check that the image handle is valid and has the loaded image protocol
  installed
- in StartImage() record the current image
- in Exit() check that the image is the current image

v2
	avoid `parent_image` may be used uninitialized

Heinrich Schuchardt (2):
  efi_loader: rearrange boottime service functions
  efi_loader: parameter checks in StartImage and Exit()

 lib/efi_loader/efi_boottime.c | 245 +++++++++++++++++++---------------
 1 file changed, 136 insertions(+), 109 deletions(-)

--
2.20.1

[U-Boot] [PATCH v2 1/2] efi_loader: rearrange boottime service functions

[Heinrich Schuchardt]

To avoid forward declarations move efi_start_image() and efi_exit() down.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	no change
---
 lib/efi_loader/efi_boottime.c | 218 +++++++++++++++++-----------------
 1 file changed, 109 insertions(+), 109 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 7038246902..6aac8391c5 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1744,115 +1744,6 @@ error:
 	return EFI_EXIT(ret);
 }

-/**
- * efi_start_image() - call the entry point of an image
- * @image_handle:   handle of the image
- * @exit_data_size: size of the buffer
- * @exit_data:      buffer to receive the exit data of the called image
- *
- * This function implements the StartImage service.
- *
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
- *
- * Return: status code
- */
-efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
-				    efi_uintn_t *exit_data_size,
-				    u16 **exit_data)
-{
-	struct efi_loaded_image_obj *image_obj =
-		(struct efi_loaded_image_obj *)image_handle;
-	efi_status_t ret;
-
-	EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size, exit_data);
-
-	efi_is_direct_boot = false;
-
-	/* call the image! */
-	if (setjmp(&image_obj->exit_jmp)) {
-		/*
-		 * We called the entry point of the child image with EFI_CALL
-		 * in the lines below. The child image called the Exit() boot
-		 * service efi_exit() which executed the long jump that brought
-		 * us to the current line. This implies that the second half
-		 * of the EFI_CALL macro has not been executed.
-		 */
-#ifdef CONFIG_ARM
-		/*
-		 * efi_exit() called efi_restore_gd(). We have to undo this
-		 * otherwise __efi_entry_check() will put the wrong value into
-		 * app_gd.
-		 */
-		gd = app_gd;
-#endif
-		/*
-		 * To get ready to call EFI_EXIT below we have to execute the
-		 * missed out steps of EFI_CALL.
-		 */
-		assert(__efi_entry_check());
-		debug("%sEFI: %lu returned by started image\n",
-		      __efi_nesting_dec(),
-		      (unsigned long)((uintptr_t)image_obj->exit_status &
-				      ~EFI_ERROR_MASK));
-		return EFI_EXIT(image_obj->exit_status);
-	}
-
-	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
-
-	/*
-	 * Usually UEFI applications call Exit() instead of returning.
-	 * But because the world doesn't consist of ponies and unicorns,
-	 * we're happy to emulate that behavior on behalf of a payload
-	 * that forgot.
-	 */
-	return EFI_CALL(systab.boottime->exit(image_handle, ret, 0, NULL));
-}
-
-/**
- * efi_exit() - leave an EFI application or driver
- * @image_handle:   handle of the application or driver that is exiting
- * @exit_status:    status code
- * @exit_data_size: size of the buffer in bytes
- * @exit_data:      buffer with data describing an error
- *
- * This function implements the Exit service.
- *
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
- *
- * Return: status code
- */
-static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
-				    efi_status_t exit_status,
-				    efi_uintn_t exit_data_size,
-				    u16 *exit_data)
-{
-	/*
-	 * TODO: We should call the unload procedure of the loaded
-	 *	 image protocol.
-	 */
-	struct efi_loaded_image_obj *image_obj =
-		(struct efi_loaded_image_obj *)image_handle;
-
-	EFI_ENTRY("%p, %ld, %zu, %p", image_handle, exit_status,
-		  exit_data_size, exit_data);
-
-	/* Make sure entry/exit counts for EFI world cross-overs match */
-	EFI_EXIT(exit_status);
-
-	/*
-	 * But longjmp out with the U-Boot gd, not the application's, as
-	 * the other end is a setjmp call inside EFI context.
-	 */
-	efi_restore_gd();
-
-	image_obj->exit_status = exit_status;
-	longjmp(&image_obj->exit_jmp, 1);
-
-	panic("EFI application exited");
-}
-
 /**
  * efi_unload_image() - unload an EFI image
  * @image_handle: handle of the image to be unloaded
@@ -2720,6 +2611,115 @@ out:
 	return EFI_EXIT(r);
 }

+/**
+ * efi_start_image() - call the entry point of an image
+ * @image_handle:   handle of the image
+ * @exit_data_size: size of the buffer
+ * @exit_data:      buffer to receive the exit data of the called image
+ *
+ * This function implements the StartImage service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * Return: status code
+ */
+efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
+				    efi_uintn_t *exit_data_size,
+				    u16 **exit_data)
+{
+	struct efi_loaded_image_obj *image_obj =
+		(struct efi_loaded_image_obj *)image_handle;
+	efi_status_t ret;
+
+	EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size, exit_data);
+
+	efi_is_direct_boot = false;
+
+	/* call the image! */
+	if (setjmp(&image_obj->exit_jmp)) {
+		/*
+		 * We called the entry point of the child image with EFI_CALL
+		 * in the lines below. The child image called the Exit() boot
+		 * service efi_exit() which executed the long jump that brought
+		 * us to the current line. This implies that the second half
+		 * of the EFI_CALL macro has not been executed.
+		 */
+#ifdef CONFIG_ARM
+		/*
+		 * efi_exit() called efi_restore_gd(). We have to undo this
+		 * otherwise __efi_entry_check() will put the wrong value into
+		 * app_gd.
+		 */
+		gd = app_gd;
+#endif
+		/*
+		 * To get ready to call EFI_EXIT below we have to execute the
+		 * missed out steps of EFI_CALL.
+		 */
+		assert(__efi_entry_check());
+		debug("%sEFI: %lu returned by started image\n",
+		      __efi_nesting_dec(),
+		      (unsigned long)((uintptr_t)image_obj->exit_status &
+				      ~EFI_ERROR_MASK));
+		return EFI_EXIT(image_obj->exit_status);
+	}
+
+	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
+
+	/*
+	 * Usually UEFI applications call Exit() instead of returning.
+	 * But because the world doesn't consist of ponies and unicorns,
+	 * we're happy to emulate that behavior on behalf of a payload
+	 * that forgot.
+	 */
+	return EFI_CALL(systab.boottime->exit(image_handle, ret, 0, NULL));
+}
+
+/**
+ * efi_exit() - leave an EFI application or driver
+ * @image_handle:   handle of the application or driver that is exiting
+ * @exit_status:    status code
+ * @exit_data_size: size of the buffer in bytes
+ * @exit_data:      buffer with data describing an error
+ *
+ * This function implements the Exit service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * Return: status code
+ */
+static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
+				    efi_status_t exit_status,
+				    efi_uintn_t exit_data_size,
+				    u16 *exit_data)
+{
+	/*
+	 * TODO: We should call the unload procedure of the loaded
+	 *	 image protocol.
+	 */
+	struct efi_loaded_image_obj *image_obj =
+		(struct efi_loaded_image_obj *)image_handle;
+
+	EFI_ENTRY("%p, %ld, %zu, %p", image_handle, exit_status,
+		  exit_data_size, exit_data);
+
+	/* Make sure entry/exit counts for EFI world cross-overs match */
+	EFI_EXIT(exit_status);
+
+	/*
+	 * But longjmp out with the U-Boot gd, not the application's, as
+	 * the other end is a setjmp call inside EFI context.
+	 */
+	efi_restore_gd();
+
+	image_obj->exit_status = exit_status;
+	longjmp(&image_obj->exit_jmp, 1);
+
+	panic("EFI application exited");
+}
+
 /**
  * efi_handle_protocol() - get interface of a protocol on a handle
  * @handle:             handle on which the protocol shall be opened
--
2.20.1

[U-Boot] [PATCH v2 2/2] efi_loader: parameter checks in StartImage and Exit()

[Heinrich Schuchardt]

Add parameter checks in the StartImage() and Exit() boottime services:
- check that the image handle is valid and has the loaded image protocol
  installed
- in StartImage() record the current image
- in Exit() check that the image is the current image

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	avoid `parent_image` may be used uninitialized
---
 lib/efi_loader/efi_boottime.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 6aac8391c5..970c01614e 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -26,6 +26,9 @@ LIST_HEAD(efi_obj_list);
 /* List of all events */
 LIST_HEAD(efi_events);

+/* Handle of the currently executing image */
+static efi_handle_t current_image;
+
 /*
  * If we're running on nasty systems (32bit ARM booting into non-EFI Linux)
  * we need to do trickery with caches. Since we don't want to break the EFI
@@ -2631,9 +2634,18 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 	struct efi_loaded_image_obj *image_obj =
 		(struct efi_loaded_image_obj *)image_handle;
 	efi_status_t ret;
+	void *info;
+	efi_handle_t parent_image = current_image;

 	EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size, exit_data);

+	/* Check parameters */
+	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
+					 &info, NULL, NULL,
+					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
+	if (ret != EFI_SUCCESS)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
 	efi_is_direct_boot = false;

 	/* call the image! */
@@ -2662,9 +2674,11 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 		      __efi_nesting_dec(),
 		      (unsigned long)((uintptr_t)image_obj->exit_status &
 				      ~EFI_ERROR_MASK));
+		current_image = parent_image;
 		return EFI_EXIT(image_obj->exit_status);
 	}

+	current_image = image_handle;
 	ret = EFI_CALL(image_obj->entry(image_handle, &systab));

 	/*
@@ -2699,12 +2713,23 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
 	 * TODO: We should call the unload procedure of the loaded
 	 *	 image protocol.
 	 */
+	efi_status_t ret;
+	void *info;
 	struct efi_loaded_image_obj *image_obj =
 		(struct efi_loaded_image_obj *)image_handle;

 	EFI_ENTRY("%p, %ld, %zu, %p", image_handle, exit_status,
 		  exit_data_size, exit_data);

+	/* Check parameters */
+	if (image_handle != current_image)
+		goto out;
+	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
+					 &info, NULL, NULL,
+					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
+	if (ret != EFI_SUCCESS)
+		goto out;
+
 	/* Make sure entry/exit counts for EFI world cross-overs match */
 	EFI_EXIT(exit_status);

@@ -2718,6 +2743,8 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
 	longjmp(&image_obj->exit_jmp, 1);

 	panic("EFI application exited");
+out:
+	return EFI_EXIT(EFI_INVALID_PARAMETER);
 }

 /**
--
2.20.1

[U-Boot] [PATCH v2 2/2] efi_loader: parameter checks in StartImage and Exit()

[Takahiro Akashi]

On Fri, Apr 05, 2019 at 03:52:58AM +0200, Heinrich Schuchardt wrote:
> Add parameter checks in the StartImage() and Exit() boottime services:
> - check that the image handle is valid and has the loaded image protocol
>   installed
> - in StartImage() record the current image
> - in Exit() check that the image is the current image
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
> 	avoid `parent_image` may be used uninitialized
> ---
>  lib/efi_loader/efi_boottime.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 6aac8391c5..970c01614e 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -26,6 +26,9 @@ LIST_HEAD(efi_obj_list);
>  /* List of all events */
>  LIST_HEAD(efi_events);
> 
> +/* Handle of the currently executing image */
> +static efi_handle_t current_image;
> +
>  /*
>   * If we're running on nasty systems (32bit ARM booting into non-EFI Linux)
>   * we need to do trickery with caches. Since we don't want to break the EFI
> @@ -2631,9 +2634,18 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>  	struct efi_loaded_image_obj *image_obj =
>  		(struct efi_loaded_image_obj *)image_handle;
>  	efi_status_t ret;
> +	void *info;
> +	efi_handle_t parent_image = current_image;
> 
>  	EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size, exit_data);
> 
> +	/* Check parameters */
> +	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
> +					 &info, NULL, NULL,
> +					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
> +	if (ret != EFI_SUCCESS)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
>  	efi_is_direct_boot = false;
> 
>  	/* call the image! */
> @@ -2662,9 +2674,11 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>  		      __efi_nesting_dec(),
>  		      (unsigned long)((uintptr_t)image_obj->exit_status &
>  				      ~EFI_ERROR_MASK));
> +		current_image = parent_image;
>  		return EFI_EXIT(image_obj->exit_status);
>  	}
> 
> +	current_image = image_handle;
>  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> 
>  	/*
> @@ -2699,12 +2713,23 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>  	 * TODO: We should call the unload procedure of the loaded
>  	 *	 image protocol.
>  	 */
> +	efi_status_t ret;
> +	void *info;
>  	struct efi_loaded_image_obj *image_obj =
>  		(struct efi_loaded_image_obj *)image_handle;
> 
>  	EFI_ENTRY("%p, %ld, %zu, %p", image_handle, exit_status,
>  		  exit_data_size, exit_data);
> 
> +	/* Check parameters */
> +	if (image_handle != current_image)
> +		goto out;
> +	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
> +					 &info, NULL, NULL,
> +					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));

Why do we need this protocol check again?

-Takahiro Akashi

> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
>  	/* Make sure entry/exit counts for EFI world cross-overs match */
>  	EFI_EXIT(exit_status);
> 
> @@ -2718,6 +2743,8 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>  	longjmp(&image_obj->exit_jmp, 1);
> 
>  	panic("EFI application exited");
> +out:
> +	return EFI_EXIT(EFI_INVALID_PARAMETER);
>  }
> 
>  /**
> --
> 2.20.1
> 

[U-Boot] [PATCH v2 2/2] efi_loader: parameter checks in StartImage and Exit()

[Takahiro Akashi]

On Fri, Apr 05, 2019 at 03:52:58AM +0200, Heinrich Schuchardt wrote:
> Add parameter checks in the StartImage() and Exit() boottime services:
> - check that the image handle is valid and has the loaded image protocol
>   installed
> - in StartImage() record the current image
> - in Exit() check that the image is the current image

Does this check logic work for a case of nested calls of StartImage() at all?

-Takahiro Akashi

> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
> 	avoid `parent_image` may be used uninitialized
> ---
>  lib/efi_loader/efi_boottime.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 6aac8391c5..970c01614e 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -26,6 +26,9 @@ LIST_HEAD(efi_obj_list);
>  /* List of all events */
>  LIST_HEAD(efi_events);
> 
> +/* Handle of the currently executing image */
> +static efi_handle_t current_image;
> +
>  /*
>   * If we're running on nasty systems (32bit ARM booting into non-EFI Linux)
>   * we need to do trickery with caches. Since we don't want to break the EFI
> @@ -2631,9 +2634,18 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>  	struct efi_loaded_image_obj *image_obj =
>  		(struct efi_loaded_image_obj *)image_handle;
>  	efi_status_t ret;
> +	void *info;
> +	efi_handle_t parent_image = current_image;
> 
>  	EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size, exit_data);
> 
> +	/* Check parameters */
> +	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
> +					 &info, NULL, NULL,
> +					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
> +	if (ret != EFI_SUCCESS)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
>  	efi_is_direct_boot = false;
> 
>  	/* call the image! */
> @@ -2662,9 +2674,11 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>  		      __efi_nesting_dec(),
>  		      (unsigned long)((uintptr_t)image_obj->exit_status &
>  				      ~EFI_ERROR_MASK));
> +		current_image = parent_image;
>  		return EFI_EXIT(image_obj->exit_status);
>  	}
> 
> +	current_image = image_handle;
>  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> 
>  	/*
> @@ -2699,12 +2713,23 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>  	 * TODO: We should call the unload procedure of the loaded
>  	 *	 image protocol.
>  	 */
> +	efi_status_t ret;
> +	void *info;
>  	struct efi_loaded_image_obj *image_obj =
>  		(struct efi_loaded_image_obj *)image_handle;
> 
>  	EFI_ENTRY("%p, %ld, %zu, %p", image_handle, exit_status,
>  		  exit_data_size, exit_data);
> 
> +	/* Check parameters */
> +	if (image_handle != current_image)
> +		goto out;
> +	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
> +					 &info, NULL, NULL,
> +					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
>  	/* Make sure entry/exit counts for EFI world cross-overs match */
>  	EFI_EXIT(exit_status);
> 
> @@ -2718,6 +2743,8 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>  	longjmp(&image_obj->exit_jmp, 1);
> 
>  	panic("EFI application exited");
> +out:
> +	return EFI_EXIT(EFI_INVALID_PARAMETER);
>  }
> 
>  /**
> --
> 2.20.1
>