[PATCH 0/3] EFI: Miscellaneous capsule update fixes

[PATCH 0/3] 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.

Sughosh Ganu (3):
  EFI: Populate descriptor_count value only when image_info_size is not
    zero
  EFI: Do not read OsIndications variable if EFI_IGNORE_OSINDICATIONS is
    enabled
  EFI: Update the documentation to reflect the correct value of
    OsIndications

 doc/develop/uefi/uefi.rst     | 2 +-
 lib/efi_loader/efi_capsule.c  | 5 +++--
 lib/efi_loader/efi_firmware.c | 5 ++---
 3 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/3] 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>
---
 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 27953fe769..6290ca0190 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 2/3] EFI: Do not read OsIndications variable if 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. Move the
check to see if the config symbol is enabled at the beginning of the
function. If the config is enabled, return a success code without
checking for the existence of the variable.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 lib/efi_loader/efi_capsule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index c76a5f3570..0ea21dc168 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -1062,6 +1062,9 @@ static efi_status_t check_run_capsules(void)
 	efi_uintn_t size;
 	efi_status_t r;
 
+	if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS))
+		return EFI_SUCCESS;
+
 	size = sizeof(os_indications);
 	r = efi_get_variable_int(u"OsIndications", &efi_global_variable_guid,
 				 NULL, &size, &os_indications, NULL);
@@ -1082,8 +1085,6 @@ static efi_status_t check_run_capsules(void)
 		if (r != EFI_SUCCESS)
 			log_err("Setting %ls failed\n", L"OsIndications");
 		return EFI_SUCCESS;
-	} else if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) {
-		return EFI_SUCCESS;
 	} else  {
 		return EFI_NOT_FOUND;
 	}
-- 
2.25.1

[PATCH 3/3] 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>
---
 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

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

[Heinrich Schuchardt]

On 5/31/22 09:15, Sughosh Ganu 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>

> ---
>   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 27953fe769..6290ca0190 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 */

Re: [PATCH 2/3] EFI: Do not read OsIndications variable if EFI_IGNORE_OSINDICATIONS is enabled

[Heinrich Schuchardt]

On 5/31/22 09:15, Sughosh Ganu wrote:
> 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. Move the
> check to see if the config symbol is enabled at the beginning of the
> function. If the config is enabled, return a success code without
> checking for the existence of the variable.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   lib/efi_loader/efi_capsule.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index c76a5f3570..0ea21dc168 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -1062,6 +1062,9 @@ static efi_status_t check_run_capsules(void)
>   	efi_uintn_t size;
>   	efi_status_t r;
>
> +	if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS))
> +		return EFI_SUCCESS;
> +

If CONFIG_EFI_IGNORE_OSINDICATIONS=y it is correct to ignore a missing
OsIndications variable.

But I think we should stay as close as possible to the UEFI specification:

If the variable exists and the flag
EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED is set, we should
remove the flag even if CONFIG_EFI_IGNORE_OSINDICATIONS=y.

Please, have a look at the configuration symbols
EFI_CAPSULE_ON_DISK_EARLY and EFI_SETUP_EARLY.

As block devices now can be added to the UEFI sub-system after
initialization of the same shouldn't we always initialize the sub-system
early. And shouldn't we always consider capsules before distro-boot,
U-Boot menu, or entering the U-Boot shell?

Best regards

Heinrich

>   	size = sizeof(os_indications);
>   	r = efi_get_variable_int(u"OsIndications", &efi_global_variable_guid,
>   				 NULL, &size, &os_indications, NULL);
> @@ -1082,8 +1085,6 @@ static efi_status_t check_run_capsules(void)
>   		if (r != EFI_SUCCESS)
>   			log_err("Setting %ls failed\n", L"OsIndications");
>   		return EFI_SUCCESS;
> -	} else if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) {
> -		return EFI_SUCCESS;
>   	} else  {
>   		return EFI_NOT_FOUND;
>   	}

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

[Heinrich Schuchardt]

On 5/31/22 09:15, Sughosh Ganu wrote:
> 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>

> ---
>   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

Re: [PATCH 2/3] EFI: Do not read OsIndications variable if EFI_IGNORE_OSINDICATIONS is enabled

[Sughosh Ganu]

hi Heinrich,

On Wed, 1 Jun 2022 at 11:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 5/31/22 09:15, Sughosh Ganu wrote:
> > 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. Move the
> > check to see if the config symbol is enabled at the beginning of the
> > function. If the config is enabled, return a success code without
> > checking for the existence of the variable.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   lib/efi_loader/efi_capsule.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index c76a5f3570..0ea21dc168 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -1062,6 +1062,9 @@ static efi_status_t check_run_capsules(void)
> >       efi_uintn_t size;
> >       efi_status_t r;
> >
> > +     if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS))
> > +             return EFI_SUCCESS;
> > +
>
> If CONFIG_EFI_IGNORE_OSINDICATIONS=y it is correct to ignore a missing
> OsIndications variable.
>
> But I think we should stay as close as possible to the UEFI specification:
>
> If the variable exists and the flag
> EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED is set, we should
> remove the flag even if CONFIG_EFI_IGNORE_OSINDICATIONS=y.

Okay. I get your point. Will make the change to not return
EFI_NOT_FOUND when the OsIndications variable is not set with
CONFIG_EFI_IGNORE_OSINDICATIONS=y. The subsequent code will clear the
EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED bit if the variable
is found irrespective of the value of the above config.

>
> Please, have a look at the configuration symbols
> EFI_CAPSULE_ON_DISK_EARLY and EFI_SETUP_EARLY.
>
> As block devices now can be added to the UEFI sub-system after
> initialization of the same shouldn't we always initialize the sub-system
> early. And shouldn't we always consider capsules before distro-boot,
> U-Boot menu, or entering the U-Boot shell?

I believe that can be done by enabling CONFIG_EFI_CAPSULE_ON_DISK_EARLY.

-sughosh

>
> Best regards
>
> Heinrich
>
> >       size = sizeof(os_indications);
> >       r = efi_get_variable_int(u"OsIndications", &efi_global_variable_guid,
> >                                NULL, &size, &os_indications, NULL);
> > @@ -1082,8 +1085,6 @@ static efi_status_t check_run_capsules(void)
> >               if (r != EFI_SUCCESS)
> >                       log_err("Setting %ls failed\n", L"OsIndications");
> >               return EFI_SUCCESS;
> > -     } else if (IS_ENABLED(CONFIG_EFI_IGNORE_OSINDICATIONS)) {
> > -             return EFI_SUCCESS;
> >       } else  {
> >               return EFI_NOT_FOUND;
> >       }

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

[Ilias Apalodimas]

On Wed, 1 Jun 2022 at 09:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 5/31/22 09:15, Sughosh Ganu wrote:
> > 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>
>
> > ---
> >   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
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

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

[Ilias Apalodimas]

On Wed, 1 Jun 2022 at 08:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 5/31/22 09:15, Sughosh Ganu 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>
>
> > ---
> >   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 27953fe769..6290ca0190 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 */
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>