[PATCH v2 0/4] EFI: Miscellaneous capsule update fixes

[PATCH v2 0/4] EFI: Miscellaneous capsule update fixes

[Sughosh Ganu]

The following set of patches fix separate issues relating to the
capsule update code.

The first patch moves the setting of the descriptor_count parameter to
the GetImageInfo function after assertion of a non zero image
descriptor buffer.

The second patch enables capsule update to proceed even when the
OsIndications variable is not set with EFI_IGNORE_OSINDICATIONS config
enabled.

The third patch updates the capsule update documentation to highlight
the 64 bit value of OsIndications that needs to be set.

The fourth patch defines a common function for the GetImageInfo
functionality for both FIT and raw image FMP instances.

Sughosh Ganu (4):
  EFI: Populate descriptor_count value only when image_info_size is not
    zero
  EFI: Do not consider OsIndications variable if
    CONFIG_EFI_IGNORE_OSINDICATIONS is enabled
  EFI: Update the documentation to reflect the correct value of
    OsIndications
  EFI: FMP: Use a common GetImageInfo function for FIT and raw images

 doc/develop/uefi/uefi.rst     |  2 +-
 lib/efi_loader/efi_capsule.c  |  7 +--
 lib/efi_loader/efi_firmware.c | 85 +++++++----------------------------
 3 files changed, 21 insertions(+), 73 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] EFI: Populate descriptor_count value only when image_info_size is not zero

[Sughosh Ganu]

The GetImageInfo function of the Firmware Mangement Protocol(FMP) gets
called initially to query the size of the image descriptor array that
would have to be allocated. During this call, the rest of the function
arguments, specifically pointers might be passed as NULL. Do not
populate the descriptor_count value before it is known that the call
to GetImageInfo has been made with the allocated buffer for the image
descriptors.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---

Changes since V1: None

 lib/efi_loader/efi_firmware.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index fe4e084106..9cdefab41f 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -130,9 +130,6 @@ static efi_status_t efi_fill_image_desc_array(
 	struct efi_fw_image *fw_array;
 	int i;
 
-	fw_array = update_info.images;
-	*descriptor_count = num_image_type_guids;
-
 	total_size = sizeof(*image_info) * num_image_type_guids;
 
 	if (*image_info_size < total_size) {
@@ -142,6 +139,8 @@ static efi_status_t efi_fill_image_desc_array(
 	}
 	*image_info_size = total_size;
 
+	fw_array = update_info.images;
+	*descriptor_count = num_image_type_guids;
 	*descriptor_version = EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION;
 	*descriptor_size = sizeof(*image_info);
 	*package_version = 0xffffffff; /* not supported */
-- 
2.25.1

[PATCH v2 2/4] EFI: Do not consider OsIndications variable if CONFIG_EFI_IGNORE_OSINDICATIONS is enabled

[Sughosh Ganu]

The EFI_IGNORE_OSINDICATIONS config symbol was introduced as a
mechanism to have capsule updates work even on platforms where the
SetVariable runtime service was not supported. The current logic
requires the OsIndications variable to have been set to a 64 bit value
even when the EFI_IGNORE_OSINDICATIONS config is enabled. Return an
error code on not being able to read the variable only when
EFI_IGNORE_OSINDICATIONS is not enabled.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V1:
* Read the OsIndications variable, if present and clear the
  EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED flag even if the
  CONFIG_EFI_IGNORE_OSINDICATIONS config is enabled.

 lib/efi_loader/efi_capsule.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index c76a5f3570..a6b98f066a 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -1058,14 +1058,15 @@ static void efi_capsule_scan_done(void)
  */
 static efi_status_t check_run_capsules(void)
 {
-	u64 os_indications;
+	u64 os_indications = 0x0;
 	efi_uintn_t size;
 	efi_status_t r;
 
 	size = sizeof(os_indications);
 	r = efi_get_variable_int(u"OsIndications", &efi_global_variable_guid,
 				 NULL, &size, &os_indications, NULL);
-	if (r != EFI_SUCCESS || size != sizeof(os_indications))
+	if (!IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS) &&
+	    (r != EFI_SUCCESS || size != sizeof(os_indications)))
 		return EFI_NOT_FOUND;
 
 	if (os_indications &
@@ -1084,7 +1085,7 @@ static efi_status_t check_run_capsules(void)
 		return EFI_SUCCESS;
 	} else if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) {
 		return EFI_SUCCESS;
-	} else  {
+	} else {
 		return EFI_NOT_FOUND;
 	}
 }
-- 
2.25.1

[PATCH v2 3/4] EFI: Update the documentation to reflect the correct value of OsIndications

[Sughosh Ganu]

The OsIndications is a 64 bit variable, and the current code expects
the value of the variable to be 64 bit. Update the documentation to
reflect this fact.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---

Changes since V1: None

 doc/develop/uefi/uefi.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
index 753a4e5e29..941e427093 100644
--- a/doc/develop/uefi/uefi.rst
+++ b/doc/develop/uefi/uefi.rst
@@ -326,7 +326,7 @@ bit in OsIndications variable with
 
 .. code-block:: console
 
-    => setenv -e -nv -bs -rt -v OsIndications =0x04
+    => setenv -e -nv -bs -rt -v OsIndications =0x0000000000000004
 
 Since U-boot doesn't currently support SetVariable at runtime, its value
 won't be taken over across the reboot. If this is the case, you can skip
-- 
2.25.1

[PATCH v2 4/4] EFI: FMP: Use a common GetImageInfo function for FIT and raw images

[Sughosh Ganu]

The GetImageInfo function definitions for the FIT images and raw
images are the same. Use a common function for the both the Firmware
Management Protocol(FMP) instances for raw and FIT images.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V1: None

 lib/efi_loader/efi_firmware.c | 80 ++++++-----------------------------
 1 file changed, 14 insertions(+), 66 deletions(-)

diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index 9cdefab41f..0ffee3fb1f 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -177,18 +177,8 @@ static efi_status_t efi_fill_image_desc_array(
 	return EFI_SUCCESS;
 }
 
-#ifdef CONFIG_EFI_CAPSULE_FIRMWARE_FIT
-/*
- * This FIRMWARE_MANAGEMENT_PROTOCOL driver provides a firmware update
- * method with existing FIT image format, and handles
- *   - multiple regions of firmware via DFU
- * but doesn't support
- *   - versioning of firmware image
- *   - package information
- */
-
 /**
- * efi_firmware_fit_get_image_info - return information about the current
+ * efi_firmware_get_image_info - return information about the current
  *				     firmware image
  * @this:			Protocol instance
  * @image_info_size:		Size of @image_info
@@ -206,7 +196,7 @@ static efi_status_t efi_fill_image_desc_array(
  * Return		status code
  */
 static
-efi_status_t EFIAPI efi_firmware_fit_get_image_info(
+efi_status_t EFIAPI efi_firmware_get_image_info(
 	struct efi_firmware_management_protocol *this,
 	efi_uintn_t *image_info_size,
 	struct efi_firmware_image_descriptor *image_info,
@@ -239,6 +229,16 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info(
 	return EFI_EXIT(ret);
 }
 
+#ifdef CONFIG_EFI_CAPSULE_FIRMWARE_FIT
+/*
+ * This FIRMWARE_MANAGEMENT_PROTOCOL driver provides a firmware update
+ * method with existing FIT image format, and handles
+ *   - multiple regions of firmware via DFU
+ * but doesn't support
+ *   - versioning of firmware image
+ *   - package information
+ */
+
 /**
  * efi_firmware_fit_set_image - update the firmware image
  * @this:		Protocol instance
@@ -278,7 +278,7 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
 }
 
 const struct efi_firmware_management_protocol efi_fmp_fit = {
-	.get_image_info = efi_firmware_fit_get_image_info,
+	.get_image_info = efi_firmware_get_image_info,
 	.get_image = efi_firmware_get_image_unsupported,
 	.set_image = efi_firmware_fit_set_image,
 	.check_image = efi_firmware_check_image_unsupported,
@@ -293,58 +293,6 @@ const struct efi_firmware_management_protocol efi_fmp_fit = {
  * method with raw data.
  */
 
-/**
- * efi_firmware_raw_get_image_info - return information about the current
- *				     firmware image
- * @this:			Protocol instance
- * @image_info_size:		Size of @image_info
- * @image_info:			Image information
- * @descriptor_version:		Pointer to version number
- * @descriptor_count:		Pointer to number of descriptors
- * @descriptor_size:		Pointer to descriptor size
- * @package_version:		Package version
- * @package_version_name:	Package version's name
- *
- * Return information bout the current firmware image in @image_info.
- * @image_info will consist of a number of descriptors.
- * Each descriptor will be created based on "dfu_alt_info" variable.
- *
- * Return		status code
- */
-static
-efi_status_t EFIAPI efi_firmware_raw_get_image_info(
-	struct efi_firmware_management_protocol *this,
-	efi_uintn_t *image_info_size,
-	struct efi_firmware_image_descriptor *image_info,
-	u32 *descriptor_version,
-	u8 *descriptor_count,
-	efi_uintn_t *descriptor_size,
-	u32 *package_version,
-	u16 **package_version_name)
-{
-	efi_status_t ret = EFI_SUCCESS;
-
-	EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this,
-		  image_info_size, image_info,
-		  descriptor_version, descriptor_count, descriptor_size,
-		  package_version, package_version_name);
-
-	if (!image_info_size)
-		return EFI_EXIT(EFI_INVALID_PARAMETER);
-
-	if (*image_info_size &&
-	    (!image_info || !descriptor_version || !descriptor_count ||
-	     !descriptor_size || !package_version || !package_version_name))
-		return EFI_EXIT(EFI_INVALID_PARAMETER);
-
-	ret = efi_fill_image_desc_array(image_info_size, image_info,
-					descriptor_version, descriptor_count,
-					descriptor_size, package_version,
-					package_version_name);
-
-	return EFI_EXIT(ret);
-}
-
 /**
  * efi_firmware_raw_set_image - update the firmware image
  * @this:		Protocol instance
@@ -430,7 +378,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
 }
 
 const struct efi_firmware_management_protocol efi_fmp_raw = {
-	.get_image_info = efi_firmware_raw_get_image_info,
+	.get_image_info = efi_firmware_get_image_info,
 	.get_image = efi_firmware_get_image_unsupported,
 	.set_image = efi_firmware_raw_set_image,
 	.check_image = efi_firmware_check_image_unsupported,
-- 
2.25.1

Re: [PATCH v2 1/4] EFI: Populate descriptor_count value only when image_info_size is not zero

[Peter Griffin]

Hi Sughosh,

On Wed, 1 Jun 2022 at 19:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:

> The GetImageInfo function of the Firmware Mangement Protocol(FMP) gets
> called initially to query the size of the image descriptor array that
> would have to be allocated. During this call, the rest of the function
> arguments, specifically pointers might be passed as NULL. Do not
> populate the descriptor_count value before it is known that the call
> to GetImageInfo has been made with the allocated buffer for the image
> descriptors.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>

This patch solves the hang issue I observed on master with
CapsuleApp.efi when doing

FS5:EFI/BOOT/app/CapsuleApp.efi -P

Which is part of the SystemReady IR ACS compliance suite.
Tested on a RockPi4b board.

Tested-by: Peter Griffin <peter.griffin@linaro.org>

Peter


> Changes since V1: None
>
>  lib/efi_loader/efi_firmware.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index fe4e084106..9cdefab41f 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -130,9 +130,6 @@ static efi_status_t efi_fill_image_desc_array(
>         struct efi_fw_image *fw_array;
>         int i;
>
> -       fw_array = update_info.images;
> -       *descriptor_count = num_image_type_guids;
> -
>         total_size = sizeof(*image_info) * num_image_type_guids;
>
>         if (*image_info_size < total_size) {
> @@ -142,6 +139,8 @@ static efi_status_t efi_fill_image_desc_array(
>         }
>         *image_info_size = total_size;
>
> +       fw_array = update_info.images;
> +       *descriptor_count = num_image_type_guids;
>         *descriptor_version = EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION;
>         *descriptor_size = sizeof(*image_info);
>         *package_version = 0xffffffff; /* not supported */
> --
> 2.25.1
>
>