diff mbox series

[v3,3/4] efi_loader: check lowest supported version in capsule update

Message ID 20230320055448.12439-4-masahisa.kojima@linaro.org
State Superseded
Headers show
Series FMP versioning support | expand

Commit Message

Masahisa Kojima March 20, 2023, 5:54 a.m. UTC 
The FMP Payload Header which EDK2 capsule generation scripts
insert contains lowest supported version.
This commit reads the lowest supported version stored in the
"FmpStateXXXX" EFI non-volatile variable, then check if the
firmware version of ongoing capsule is equal or greater than
the lowest supported version.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No changes since v2

Changes in v2:
- add error message when the firmware version is lower than
  lowest supported version

 lib/efi_loader/efi_firmware.c | 50 ++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

Comments

Heinrich Schuchardt March 21, 2023, 6:42 a.m. UTC | #1 
On 3/20/23 06:54, Masahisa Kojima wrote:
> The FMP Payload Header which EDK2 capsule generation scripts
> insert contains lowest supported version.
> This commit reads the lowest supported version stored in the
> "FmpStateXXXX" EFI non-volatile variable, then check if the
> firmware version of ongoing capsule is equal or greater than
> the lowest supported version.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> No changes since v2
>
> Changes in v2:
> - add error message when the firmware version is lower than
>    lowest supported version
>
>   lib/efi_loader/efi_firmware.c | 50 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 289456ecbb..8d6e32006d 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -391,6 +391,39 @@ void efi_firmware_parse_payload_header(const void **p_image,
>   	*p_image_size = image_size;
>   }
>
> +/**
> + * efi_firmware_get_lowest_supported_version - get the lowest supported version
> + * @image_index:	image_index
> + *
> + * Get the lowest supported version from FmpStateXXXX variable.
> + *
> + * Return:		lowest supported version, return 0 if reading FmpStateXXXX
> + *			variable failed
> + */
> +static
> +u32 efi_firmware_get_lowest_supported_version(u8 image_index)
> +{
> +	u16 varname[13]; /* u"FmpStateXXXX" */
> +	efi_status_t ret;
> +	efi_uintn_t size;
> +	efi_guid_t *image_type_id;
> +	struct fmp_state var_state = { 0 };
> +
> +	image_type_id = efi_firmware_get_image_type_id(image_index);
> +	if (!image_type_id)
> +		return 0;
> +
> +	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> +				image_index);
> +	size = sizeof(var_state);
> +	ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
> +				   &var_state, NULL);
> +	if (ret != EFI_SUCCESS)
> +		return 0;
> +
> +	return var_state.lowest_supported_version;
> +}
> +
>   /**
>    * efi_firmware_verify_image - verify image
>    * @p_image:		Pointer to new image
> @@ -398,7 +431,8 @@ void efi_firmware_parse_payload_header(const void **p_image,
>    * @image_index		Image index
>    * @state		Pointer to fmp state
>    *
> - * Verify the capsule file
> + * Verify the capsule authentication and check if the fw_version
> + * is equal or greater than the lowest supported version.
>    *
>    * Return:		status code
>    */
> @@ -409,10 +443,24 @@ efi_status_t efi_firmware_verify_image(const void **p_image,
>   				       struct fmp_state *state)
>   {
>   	efi_status_t ret;
> +	u32 lowest_supported_version;
>
>   	ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
>   	efi_firmware_parse_payload_header(p_image, p_image_size, state);
>
> +	/* check lowest_supported_version if capsule authentication passes */
> +	if (ret == EFI_SUCCESS) {
> +		lowest_supported_version =
> +			efi_firmware_get_lowest_supported_version(image_index);
> +		if (lowest_supported_version > state->fw_version) {
> +			printf("fw_version(%u) is too low(expected >%u). Aborting update\n",
> +			       state->fw_version, lowest_supported_version);

Please, use log_warning() or log_err() instead of printf().

The addressee is a user not a developer:

"Firmware version %u too low. Expecting >= %u"

We should have one central place where upon exit we write a message
indicating that either the capsule update was successful or failed.

"Firmware updated to version %u" : "Firmware update failed"

Best regards

Heinrich

> +			state->last_attempt_status =
> +				LAST_ATTEMPT_STATUS_ERROR_INCORRECT_VERSION;
> +			ret = EFI_INVALID_PARAMETER;
> +		}
> +	}
> +
>   	return ret;
>   }
>
Masahisa Kojima March 22, 2023, 12:36 a.m. UTC | #2 
On Tue, 21 Mar 2023 at 15:42, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/20/23 06:54, Masahisa Kojima wrote:
> > The FMP Payload Header which EDK2 capsule generation scripts
> > insert contains lowest supported version.
> > This commit reads the lowest supported version stored in the
> > "FmpStateXXXX" EFI non-volatile variable, then check if the
> > firmware version of ongoing capsule is equal or greater than
> > the lowest supported version.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > No changes since v2
> >
> > Changes in v2:
> > - add error message when the firmware version is lower than
> >    lowest supported version
> >
> >   lib/efi_loader/efi_firmware.c | 50 ++++++++++++++++++++++++++++++++++-
> >   1 file changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > index 289456ecbb..8d6e32006d 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -391,6 +391,39 @@ void efi_firmware_parse_payload_header(const void **p_image,
> >       *p_image_size = image_size;
> >   }
> >
> > +/**
> > + * efi_firmware_get_lowest_supported_version - get the lowest supported version
> > + * @image_index:     image_index
> > + *
> > + * Get the lowest supported version from FmpStateXXXX variable.
> > + *
> > + * Return:           lowest supported version, return 0 if reading FmpStateXXXX
> > + *                   variable failed
> > + */
> > +static
> > +u32 efi_firmware_get_lowest_supported_version(u8 image_index)
> > +{
> > +     u16 varname[13]; /* u"FmpStateXXXX" */
> > +     efi_status_t ret;
> > +     efi_uintn_t size;
> > +     efi_guid_t *image_type_id;
> > +     struct fmp_state var_state = { 0 };
> > +
> > +     image_type_id = efi_firmware_get_image_type_id(image_index);
> > +     if (!image_type_id)
> > +             return 0;
> > +
> > +     efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > +                             image_index);
> > +     size = sizeof(var_state);
> > +     ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
> > +                                &var_state, NULL);
> > +     if (ret != EFI_SUCCESS)
> > +             return 0;
> > +
> > +     return var_state.lowest_supported_version;
> > +}
> > +
> >   /**
> >    * efi_firmware_verify_image - verify image
> >    * @p_image:                Pointer to new image
> > @@ -398,7 +431,8 @@ void efi_firmware_parse_payload_header(const void **p_image,
> >    * @image_index             Image index
> >    * @state           Pointer to fmp state
> >    *
> > - * Verify the capsule file
> > + * Verify the capsule authentication and check if the fw_version
> > + * is equal or greater than the lowest supported version.
> >    *
> >    * Return:          status code
> >    */
> > @@ -409,10 +443,24 @@ efi_status_t efi_firmware_verify_image(const void **p_image,
> >                                      struct fmp_state *state)
> >   {
> >       efi_status_t ret;
> > +     u32 lowest_supported_version;
> >
> >       ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> >       efi_firmware_parse_payload_header(p_image, p_image_size, state);
> >
> > +     /* check lowest_supported_version if capsule authentication passes */
> > +     if (ret == EFI_SUCCESS) {
> > +             lowest_supported_version =
> > +                     efi_firmware_get_lowest_supported_version(image_index);
> > +             if (lowest_supported_version > state->fw_version) {
> > +                     printf("fw_version(%u) is too low(expected >%u). Aborting update\n",
> > +                            state->fw_version, lowest_supported_version);
>
> Please, use log_warning() or log_err() instead of printf().
>
> The addressee is a user not a developer:
>
> "Firmware version %u too low. Expecting >= %u"

OK.

>
> We should have one central place where upon exit we write a message
> indicating that either the capsule update was successful or failed.
>
> "Firmware updated to version %u" : "Firmware update failed"

Yes, I will add the log output for the user.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> > +                     state->last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_ERROR_INCORRECT_VERSION;
> > +                     ret = EFI_INVALID_PARAMETER;
> > +             }
> > +     }
> > +
> >       return ret;
> >   }
> >
>
Masahisa Kojima March 23, 2023, 3:23 a.m. UTC | #3 
On Wed, 22 Mar 2023 at 09:36, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> On Tue, 21 Mar 2023 at 15:42, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 3/20/23 06:54, Masahisa Kojima wrote:
> > > The FMP Payload Header which EDK2 capsule generation scripts
> > > insert contains lowest supported version.
> > > This commit reads the lowest supported version stored in the
> > > "FmpStateXXXX" EFI non-volatile variable, then check if the
> > > firmware version of ongoing capsule is equal or greater than
> > > the lowest supported version.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > > No changes since v2
> > >
> > > Changes in v2:
> > > - add error message when the firmware version is lower than
> > >    lowest supported version
> > >
> > >   lib/efi_loader/efi_firmware.c | 50 ++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 49 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > > index 289456ecbb..8d6e32006d 100644
> > > --- a/lib/efi_loader/efi_firmware.c
> > > +++ b/lib/efi_loader/efi_firmware.c
> > > @@ -391,6 +391,39 @@ void efi_firmware_parse_payload_header(const void **p_image,
> > >       *p_image_size = image_size;
> > >   }
> > >
> > > +/**
> > > + * efi_firmware_get_lowest_supported_version - get the lowest supported version
> > > + * @image_index:     image_index
> > > + *
> > > + * Get the lowest supported version from FmpStateXXXX variable.
> > > + *
> > > + * Return:           lowest supported version, return 0 if reading FmpStateXXXX
> > > + *                   variable failed
> > > + */
> > > +static
> > > +u32 efi_firmware_get_lowest_supported_version(u8 image_index)
> > > +{
> > > +     u16 varname[13]; /* u"FmpStateXXXX" */
> > > +     efi_status_t ret;
> > > +     efi_uintn_t size;
> > > +     efi_guid_t *image_type_id;
> > > +     struct fmp_state var_state = { 0 };
> > > +
> > > +     image_type_id = efi_firmware_get_image_type_id(image_index);
> > > +     if (!image_type_id)
> > > +             return 0;
> > > +
> > > +     efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > > +                             image_index);
> > > +     size = sizeof(var_state);
> > > +     ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
> > > +                                &var_state, NULL);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return 0;
> > > +
> > > +     return var_state.lowest_supported_version;
> > > +}
> > > +
> > >   /**
> > >    * efi_firmware_verify_image - verify image
> > >    * @p_image:                Pointer to new image
> > > @@ -398,7 +431,8 @@ void efi_firmware_parse_payload_header(const void **p_image,
> > >    * @image_index             Image index
> > >    * @state           Pointer to fmp state
> > >    *
> > > - * Verify the capsule file
> > > + * Verify the capsule authentication and check if the fw_version
> > > + * is equal or greater than the lowest supported version.
> > >    *
> > >    * Return:          status code
> > >    */
> > > @@ -409,10 +443,24 @@ efi_status_t efi_firmware_verify_image(const void **p_image,
> > >                                      struct fmp_state *state)
> > >   {
> > >       efi_status_t ret;
> > > +     u32 lowest_supported_version;
> > >
> > >       ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> > >       efi_firmware_parse_payload_header(p_image, p_image_size, state);
> > >
> > > +     /* check lowest_supported_version if capsule authentication passes */
> > > +     if (ret == EFI_SUCCESS) {
> > > +             lowest_supported_version =
> > > +                     efi_firmware_get_lowest_supported_version(image_index);
> > > +             if (lowest_supported_version > state->fw_version) {
> > > +                     printf("fw_version(%u) is too low(expected >%u). Aborting update\n",
> > > +                            state->fw_version, lowest_supported_version);
> >
> > Please, use log_warning() or log_err() instead of printf().
> >
> > The addressee is a user not a developer:
> >
> > "Firmware version %u too low. Expecting >= %u"
>
> OK.
>
> >
> > We should have one central place where upon exit we write a message
> > indicating that either the capsule update was successful or failed.
> >
> > "Firmware updated to version %u" : "Firmware update failed"
>
> Yes, I will add the log output for the user.

For failure cases, UpdateCapsule runtime service outputs the result as follows.
"Firmware update failed: <NULL>
Applying capsule Test08 failed."

So I will not add the "Firmware update failed" in FMP->SetImage() function.

Thanks,
Masahisa Kojima

>
> Thanks,
> Masahisa Kojima
>
> >
> > Best regards
> >
> > Heinrich
> >
> > > +                     state->last_attempt_status =
> > > +                             LAST_ATTEMPT_STATUS_ERROR_INCORRECT_VERSION;
> > > +                     ret = EFI_INVALID_PARAMETER;
> > > +             }
> > > +     }
> > > +
> > >       return ret;
> > >   }
> > >
> >
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index 289456ecbb..8d6e32006d 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -391,6 +391,39 @@  void efi_firmware_parse_payload_header(const void **p_image,
 	*p_image_size = image_size;
 }
 
+/**
+ * efi_firmware_get_lowest_supported_version - get the lowest supported version
+ * @image_index:	image_index
+ *
+ * Get the lowest supported version from FmpStateXXXX variable.
+ *
+ * Return:		lowest supported version, return 0 if reading FmpStateXXXX
+ *			variable failed
+ */
+static
+u32 efi_firmware_get_lowest_supported_version(u8 image_index)
+{
+	u16 varname[13]; /* u"FmpStateXXXX" */
+	efi_status_t ret;
+	efi_uintn_t size;
+	efi_guid_t *image_type_id;
+	struct fmp_state var_state = { 0 };
+
+	image_type_id = efi_firmware_get_image_type_id(image_index);
+	if (!image_type_id)
+		return 0;
+
+	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
+				image_index);
+	size = sizeof(var_state);
+	ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
+				   &var_state, NULL);
+	if (ret != EFI_SUCCESS)
+		return 0;
+
+	return var_state.lowest_supported_version;
+}
+
 /**
  * efi_firmware_verify_image - verify image
  * @p_image:		Pointer to new image
@@ -398,7 +431,8 @@  void efi_firmware_parse_payload_header(const void **p_image,
  * @image_index		Image index
  * @state		Pointer to fmp state
  *
- * Verify the capsule file
+ * Verify the capsule authentication and check if the fw_version
+ * is equal or greater than the lowest supported version.
  *
  * Return:		status code
  */
@@ -409,10 +443,24 @@  efi_status_t efi_firmware_verify_image(const void **p_image,
 				       struct fmp_state *state)
 {
 	efi_status_t ret;
+	u32 lowest_supported_version;
 
 	ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
 	efi_firmware_parse_payload_header(p_image, p_image_size, state);
 
+	/* check lowest_supported_version if capsule authentication passes */
+	if (ret == EFI_SUCCESS) {
+		lowest_supported_version =
+			efi_firmware_get_lowest_supported_version(image_index);
+		if (lowest_supported_version > state->fw_version) {
+			printf("fw_version(%u) is too low(expected >%u). Aborting update\n",
+			       state->fw_version, lowest_supported_version);
+			state->last_attempt_status =
+				LAST_ATTEMPT_STATUS_ERROR_INCORRECT_VERSION;
+			ret = EFI_INVALID_PARAMETER;
+		}
+	}
+
 	return ret;
 }