[v2,1/4] efi_loader: store firmware version into FmpState variable

Firmware version management is not implemented in the current
FMP protocol.
EDK2 reference implementation capsule generation script inserts
the FMP Payload Header right before the payload, it contains the
firmware version and lowest supported version.

This commit utilizes the FMP Payload Header, read the header and
stores the firmware version, lowest supported version,
last attempt version and last attempt status into "FmpStateXXXX"
EFI non-volatile variable. XXXX indicates the image index,
since FMP protocol handles multiple image indexes.

This change is compatible with the existing FMP implementation.
This change does not mandate the FMP Payload Header.
If no FMP Payload Header is found in the capsule file, fw_version,
lowest supported version, last attempt version and last attempt
status is 0 and this is the same behavior as existing FMP
implementation.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v2:
- modify indent

 lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
 1 file changed, 175 insertions(+), 23 deletions(-)

On Wed, Mar 01, 2023 at 06:15:19PM +0900, Masahisa Kojima wrote:
> Firmware version management is not implemented in the current
> FMP protocol.
> EDK2 reference implementation capsule generation script inserts
> the FMP Payload Header right before the payload, it contains the
> firmware version and lowest supported version.
> 
> This commit utilizes the FMP Payload Header, read the header and
> stores the firmware version, lowest supported version,
> last attempt version and last attempt status into "FmpStateXXXX"
> EFI non-volatile variable. XXXX indicates the image index,
> since FMP protocol handles multiple image indexes.
> 
> This change is compatible with the existing FMP implementation.
> This change does not mandate the FMP Payload Header.
> If no FMP Payload Header is found in the capsule file, fw_version,
> lowest supported version, last attempt version and last attempt
> status is 0 and this is the same behavior as existing FMP
> implementation.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v2:
> - modify indent
> 
>  lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
>  1 file changed, 175 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 93e2b01c07..d1afafb052 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -10,6 +10,7 @@
>  #include <charset.h>
>  #include <dfu.h>
>  #include <efi_loader.h>
> +#include <efi_variable.h>
>  #include <fwu.h>
>  #include <image.h>
>  #include <signatures.h>
> @@ -18,6 +19,12 @@
>  
>  #define FMP_PAYLOAD_HDR_SIGNATURE	SIGNATURE_32('M', 'S', 'S', '1')
>  
> +#define EFI_FMP_STATE_GUID \
> +	EFI_GUID(0x84bed885, 0x193a, 0x403f, 0xa2, 0x78, \
> +		 0xe8, 0x9e, 0x23, 0x8a, 0xd6, 0xe1)
> +
> +static const efi_guid_t efi_guid_fmp_state = EFI_FMP_STATE_GUID;
> +
>  /**
>   * struct fmp_payload_header - EDK2 header for the FMP payload
>   *
> @@ -36,6 +43,24 @@ struct fmp_payload_header {
>  	u32 lowest_supported_version;
>  };
>  
> +/**
> + * struct fmp_state - fmp firmware update state
> + *
> + * This structure describes the state of the firmware update
> + * through FMP protocol.
> + *
> + * @fw_version:			Firmware versions used
> + * @lowest_supported_version:	Lowest supported version
> + * @last_attempt_version:	Last attempt version
> + * @last_attempt_status:	Last attempt status
> + */
> +struct fmp_state {
> +	u32 fw_version;
> +	u32 lowest_supported_version;
> +	u32 last_attempt_version;
> +	u32 last_attempt_status;
> +};
> +
>  __weak void set_dfu_alt_info(char *interface, char *devstr)
>  {
>  	env_set("dfu_alt_info", update_info.dfu_string);
> @@ -182,6 +207,7 @@ static efi_status_t efi_fill_image_desc_array(
>   * efi_firmware_capsule_authenticate - authenticate the capsule if enabled
>   * @p_image:		Pointer to new image
>   * @p_image_size:	Pointer to size of new image
> + * @state		Pointer to fmp state
>   *
>   * Authenticate the capsule if authentication is enabled.
>   * The image pointer and the image size are updated in case of success.
> @@ -190,12 +216,11 @@ static efi_status_t efi_fill_image_desc_array(
>   */
>  static
>  efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> -					       efi_uintn_t *p_image_size)
> +					       efi_uintn_t *p_image_size,
> +					       struct fmp_state *state)
>  {
>  	const void *image = *p_image;
>  	efi_uintn_t image_size = *p_image_size;
> -	u32 fmp_hdr_signature;
> -	struct fmp_payload_header *header;
>  	void *capsule_payload;
>  	efi_status_t status;
>  	efi_uintn_t capsule_payload_size;
> @@ -209,8 +234,12 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
>  
>  		if (status == EFI_SECURITY_VIOLATION) {
>  			printf("Capsule authentication check failed. Aborting update\n");
> +			state->last_attempt_status =
> +				LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
>  			return status;
>  		} else if (status != EFI_SUCCESS) {
> +			state->last_attempt_status =
> +				LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
>  			return status;
>  		}
>  
> @@ -222,24 +251,124 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
>  		debug("Updating capsule without authenticating.\n");
>  	}
>  
> -	fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> -	header = (void *)image;
> +	*p_image = image;
> +	*p_image_size = image_size;
> +
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_firmware_set_fmp_state_var - set FmpStateXXXX variable
> + * @state:		Pointer to fmp state
> + * @image_index:	image index
> + * @updated:		flag to indicate firmware update is successful
> + *
> + * Update the FmpStateXXXX variable with the firmware update state.
> + *
> + * Return:		status code
> + */
> +static
> +efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index,
> +					    bool updated)
> +{
> +	u16 varname[13]; /* u"FmpStateXXXX" */
> +	efi_status_t ret;
> +	efi_uintn_t size;
> +	struct fmp_state var_state = { 0 };
> +
> +	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> +				image_index);
> +	size = sizeof(var_state);
> +	ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
> +				   &var_state, NULL);
> +	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> +		return ret;
> +
> +	/*
> +	 * When the capsule update is successful, FmpStateXXXX variable is set
> +	 * according to the fmp payload header information. If there is no fmp payload
> +	 * header in the capsule file, all values are set to 0.
> +	 * When the capsule update fails, only last attempt information of FmpStateXXXX
> +	 * variable is updated, fw_version and lowest_supported_version keep original
> +	 * value or 0(in case no FmpStateXXXX variable found).
> +	 */
> +	if (updated) {
> +		var_state.fw_version = state->fw_version;
> +		var_state.lowest_supported_version = state->lowest_supported_version;
> +		var_state.last_attempt_version = state->last_attempt_version;
> +		var_state.last_attempt_status = state->last_attempt_status;
> +	} else {
> +		var_state.last_attempt_version = state->last_attempt_version;
> +		var_state.last_attempt_status = state->last_attempt_status;
> +	}
> +
> +	ret = efi_set_variable_int(varname, &efi_guid_fmp_state,
> +				   EFI_VARIABLE_NON_VOLATILE |
> +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				   EFI_VARIABLE_RUNTIME_ACCESS,
> +				   sizeof(var_state), &var_state, false);
> +
> +	return ret;
> +}
>  
> +/**
> + * efi_firmware_parse_payload_header - parse FMP payload header
> + * @p_image:		Pointer to new image
> + * @p_image_size:	Pointer to size of new image
> + * @state		Pointer to fmp state
> + *
> + * Parse the FMP payload header and fill the fmp_state structure.
> + * If no FMP payload header is found, fmp_state structure is not updated.
> + *
> + */
> +static
> +void efi_firmware_parse_payload_header(const void **p_image,
> +				       efi_uintn_t *p_image_size,
> +				       struct fmp_state *state)
> +{
> +	const void *image = *p_image;
> +	efi_uintn_t image_size = *p_image_size;
> +	const struct fmp_payload_header *header;
> +	u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> +
> +	header = image;
>  	if (!memcmp(&header->signature, &fmp_hdr_signature,
>  		    sizeof(fmp_hdr_signature))) {
> -		/*
> -		 * When building the capsule with the scripts in
> -		 * edk2, a FMP header is inserted above the capsule
> -		 * payload. Compensate for this header to get the
> -		 * actual payload that is to be updated.
> -		 */
> +		/* FMP header is inserted above the capsule payload */
> +		state->fw_version = header->fw_version;
> +		state->lowest_supported_version = header->lowest_supported_version;
> +		state->last_attempt_version = header->fw_version;
>  		image += header->header_size;
>  		image_size -= header->header_size;
>  	}
>  
>  	*p_image = image;
>  	*p_image_size = image_size;
> -	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_firmware_verify_image - verify image
> + * @p_image:		Pointer to new image
> + * @p_image_size:	Pointer to size of new image
> + * @image_index		Image index
> + * @state		Pointer to fmp state
> + *
> + * Verify the capsule file
> + *
> + * Return:		status code
> + */
> +static
> +efi_status_t efi_firmware_verify_image(const void **p_image,
> +				       efi_uintn_t *p_image_size,
> +				       u8 image_index,
> +				       struct fmp_state *state)
> +{
> +	efi_status_t ret;
> +
> +	ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> +	efi_firmware_parse_payload_header(p_image, p_image_size, state);
> +
> +	return ret;
>  }
>  
>  /**
> @@ -330,7 +459,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	efi_status_t (*progress)(efi_uintn_t completion),
>  	u16 **abort_reason)
>  {
> +	bool updated;
>  	efi_status_t status;
> +	struct fmp_state state = { 0 };
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -338,14 +469,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	if (!image || image_index != 1)
>  		return EFI_EXIT(EFI_INVALID_PARAMETER);
>  
> -	status = efi_firmware_capsule_authenticate(&image, &image_size);
> +	status = efi_firmware_verify_image(&image, &image_size, image_index,
> +					   &state);
>  	if (status != EFI_SUCCESS)
> -		return EFI_EXIT(status);
> +		goto err;
>  
> -	if (fit_update(image))
> -		return EFI_EXIT(EFI_DEVICE_ERROR);
> +	if (fit_update(image)) {
> +		status = EFI_DEVICE_ERROR;

I think we need to add here:
		state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;

-Takahiro Akashi

> +		goto err;
> +	}
>  
> -	return EFI_EXIT(EFI_SUCCESS);
> +	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +err:
> +	updated = (status == EFI_SUCCESS) ? true : false;
> +	efi_firmware_set_fmp_state_var(&state, image_index, updated);
> +
> +	return EFI_EXIT(status);
>  }
>  
>  const struct efi_firmware_management_protocol efi_fmp_fit = {
> @@ -391,7 +530,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  	u16 **abort_reason)
>  {
>  	int ret;
> +	bool updated;
>  	efi_status_t status;
> +	struct fmp_state state = { 0 };
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -399,9 +540,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  	if (!image)
>  		return EFI_EXIT(EFI_INVALID_PARAMETER);
>  
> -	status = efi_firmware_capsule_authenticate(&image, &image_size);
> +	status = efi_firmware_verify_image(&image, &image_size, image_index,
> +					   &state);
>  	if (status != EFI_SUCCESS)
> -		return EFI_EXIT(status);
> +		goto err;
>  
>  	if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
>  		/*
> @@ -410,16 +552,26 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  		 */
>  		ret = fwu_get_image_index(&image_index);
>  		if (ret) {
> +			state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
>  			log_debug("Unable to get FWU image_index\n");
> -			return EFI_EXIT(EFI_DEVICE_ERROR);
> +			status = EFI_DEVICE_ERROR;
> +			goto err;
>  		}
>  	}
>  
>  	if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
> -			     NULL, NULL))
> -		return EFI_EXIT(EFI_DEVICE_ERROR);
> +			     NULL, NULL)) {
> +		status = EFI_DEVICE_ERROR;
> +		state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> +		goto err;
> +	}
> +
> +	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +err:
> +	updated = (status == EFI_SUCCESS) ? true : false;
> +	efi_firmware_set_fmp_state_var(&state, image_index, updated);
>  
> -	return EFI_EXIT(EFI_SUCCESS);
> +	return EFI_EXIT(status);
>  }
>  
>  const struct efi_firmware_management_protocol efi_fmp_raw = {
> -- 
> 2.17.1
>

Hi Akashi-san,

On Thu, 2 Mar 2023 at 14:09, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> On Wed, Mar 01, 2023 at 06:15:19PM +0900, Masahisa Kojima wrote:
> > Firmware version management is not implemented in the current
> > FMP protocol.
> > EDK2 reference implementation capsule generation script inserts
> > the FMP Payload Header right before the payload, it contains the
> > firmware version and lowest supported version.
> >
> > This commit utilizes the FMP Payload Header, read the header and
> > stores the firmware version, lowest supported version,
> > last attempt version and last attempt status into "FmpStateXXXX"
> > EFI non-volatile variable. XXXX indicates the image index,
> > since FMP protocol handles multiple image indexes.
> >
> > This change is compatible with the existing FMP implementation.
> > This change does not mandate the FMP Payload Header.
> > If no FMP Payload Header is found in the capsule file, fw_version,
> > lowest supported version, last attempt version and last attempt
> > status is 0 and this is the same behavior as existing FMP
> > implementation.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v2:
> > - modify indent
> >
> >  lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
> >  1 file changed, 175 insertions(+), 23 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > index 93e2b01c07..d1afafb052 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -10,6 +10,7 @@
> >  #include <charset.h>
> >  #include <dfu.h>
> >  #include <efi_loader.h>
> > +#include <efi_variable.h>
> >  #include <fwu.h>
> >  #include <image.h>
> >  #include <signatures.h>
> > @@ -18,6 +19,12 @@
> >
> >  #define FMP_PAYLOAD_HDR_SIGNATURE    SIGNATURE_32('M', 'S', 'S', '1')
> >
> > +#define EFI_FMP_STATE_GUID \
> > +     EFI_GUID(0x84bed885, 0x193a, 0x403f, 0xa2, 0x78, \
> > +              0xe8, 0x9e, 0x23, 0x8a, 0xd6, 0xe1)
> > +
> > +static const efi_guid_t efi_guid_fmp_state = EFI_FMP_STATE_GUID;
> > +
> >  /**
> >   * struct fmp_payload_header - EDK2 header for the FMP payload
> >   *
> > @@ -36,6 +43,24 @@ struct fmp_payload_header {
> >       u32 lowest_supported_version;
> >  };
> >
> > +/**
> > + * struct fmp_state - fmp firmware update state
> > + *
> > + * This structure describes the state of the firmware update
> > + * through FMP protocol.
> > + *
> > + * @fw_version:                      Firmware versions used
> > + * @lowest_supported_version:        Lowest supported version
> > + * @last_attempt_version:    Last attempt version
> > + * @last_attempt_status:     Last attempt status
> > + */
> > +struct fmp_state {
> > +     u32 fw_version;
> > +     u32 lowest_supported_version;
> > +     u32 last_attempt_version;
> > +     u32 last_attempt_status;
> > +};
> > +
> >  __weak void set_dfu_alt_info(char *interface, char *devstr)
> >  {
> >       env_set("dfu_alt_info", update_info.dfu_string);
> > @@ -182,6 +207,7 @@ static efi_status_t efi_fill_image_desc_array(
> >   * efi_firmware_capsule_authenticate - authenticate the capsule if enabled
> >   * @p_image:         Pointer to new image
> >   * @p_image_size:    Pointer to size of new image
> > + * @state            Pointer to fmp state
> >   *
> >   * Authenticate the capsule if authentication is enabled.
> >   * The image pointer and the image size are updated in case of success.
> > @@ -190,12 +216,11 @@ static efi_status_t efi_fill_image_desc_array(
> >   */
> >  static
> >  efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> > -                                            efi_uintn_t *p_image_size)
> > +                                            efi_uintn_t *p_image_size,
> > +                                            struct fmp_state *state)
> >  {
> >       const void *image = *p_image;
> >       efi_uintn_t image_size = *p_image_size;
> > -     u32 fmp_hdr_signature;
> > -     struct fmp_payload_header *header;
> >       void *capsule_payload;
> >       efi_status_t status;
> >       efi_uintn_t capsule_payload_size;
> > @@ -209,8 +234,12 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> >
> >               if (status == EFI_SECURITY_VIOLATION) {
> >                       printf("Capsule authentication check failed. Aborting update\n");
> > +                     state->last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
> >                       return status;
> >               } else if (status != EFI_SUCCESS) {
> > +                     state->last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> >                       return status;
> >               }
> >
> > @@ -222,24 +251,124 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> >               debug("Updating capsule without authenticating.\n");
> >       }
> >
> > -     fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> > -     header = (void *)image;
> > +     *p_image = image;
> > +     *p_image_size = image_size;
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * efi_firmware_set_fmp_state_var - set FmpStateXXXX variable
> > + * @state:           Pointer to fmp state
> > + * @image_index:     image index
> > + * @updated:         flag to indicate firmware update is successful
> > + *
> > + * Update the FmpStateXXXX variable with the firmware update state.
> > + *
> > + * Return:           status code
> > + */
> > +static
> > +efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index,
> > +                                         bool updated)
> > +{
> > +     u16 varname[13]; /* u"FmpStateXXXX" */
> > +     efi_status_t ret;
> > +     efi_uintn_t size;
> > +     struct fmp_state var_state = { 0 };
> > +
> > +     efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > +                             image_index);
> > +     size = sizeof(var_state);
> > +     ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
> > +                                &var_state, NULL);
> > +     if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> > +             return ret;
> > +
> > +     /*
> > +      * When the capsule update is successful, FmpStateXXXX variable is set
> > +      * according to the fmp payload header information. If there is no fmp payload
> > +      * header in the capsule file, all values are set to 0.
> > +      * When the capsule update fails, only last attempt information of FmpStateXXXX
> > +      * variable is updated, fw_version and lowest_supported_version keep original
> > +      * value or 0(in case no FmpStateXXXX variable found).
> > +      */
> > +     if (updated) {
> > +             var_state.fw_version = state->fw_version;
> > +             var_state.lowest_supported_version = state->lowest_supported_version;
> > +             var_state.last_attempt_version = state->last_attempt_version;
> > +             var_state.last_attempt_status = state->last_attempt_status;
> > +     } else {
> > +             var_state.last_attempt_version = state->last_attempt_version;
> > +             var_state.last_attempt_status = state->last_attempt_status;
> > +     }
> > +
> > +     ret = efi_set_variable_int(varname, &efi_guid_fmp_state,
> > +                                EFI_VARIABLE_NON_VOLATILE |
> > +                                EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                EFI_VARIABLE_RUNTIME_ACCESS,
> > +                                sizeof(var_state), &var_state, false);
> > +
> > +     return ret;
> > +}
> >
> > +/**
> > + * efi_firmware_parse_payload_header - parse FMP payload header
> > + * @p_image:         Pointer to new image
> > + * @p_image_size:    Pointer to size of new image
> > + * @state            Pointer to fmp state
> > + *
> > + * Parse the FMP payload header and fill the fmp_state structure.
> > + * If no FMP payload header is found, fmp_state structure is not updated.
> > + *
> > + */
> > +static
> > +void efi_firmware_parse_payload_header(const void **p_image,
> > +                                    efi_uintn_t *p_image_size,
> > +                                    struct fmp_state *state)
> > +{
> > +     const void *image = *p_image;
> > +     efi_uintn_t image_size = *p_image_size;
> > +     const struct fmp_payload_header *header;
> > +     u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> > +
> > +     header = image;
> >       if (!memcmp(&header->signature, &fmp_hdr_signature,
> >                   sizeof(fmp_hdr_signature))) {
> > -             /*
> > -              * When building the capsule with the scripts in
> > -              * edk2, a FMP header is inserted above the capsule
> > -              * payload. Compensate for this header to get the
> > -              * actual payload that is to be updated.
> > -              */
> > +             /* FMP header is inserted above the capsule payload */
> > +             state->fw_version = header->fw_version;
> > +             state->lowest_supported_version = header->lowest_supported_version;
> > +             state->last_attempt_version = header->fw_version;
> >               image += header->header_size;
> >               image_size -= header->header_size;
> >       }
> >
> >       *p_image = image;
> >       *p_image_size = image_size;
> > -     return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * efi_firmware_verify_image - verify image
> > + * @p_image:         Pointer to new image
> > + * @p_image_size:    Pointer to size of new image
> > + * @image_index              Image index
> > + * @state            Pointer to fmp state
> > + *
> > + * Verify the capsule file
> > + *
> > + * Return:           status code
> > + */
> > +static
> > +efi_status_t efi_firmware_verify_image(const void **p_image,
> > +                                    efi_uintn_t *p_image_size,
> > +                                    u8 image_index,
> > +                                    struct fmp_state *state)
> > +{
> > +     efi_status_t ret;
> > +
> > +     ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> > +     efi_firmware_parse_payload_header(p_image, p_image_size, state);
> > +
> > +     return ret;
> >  }
> >
> >  /**
> > @@ -330,7 +459,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
> >       efi_status_t (*progress)(efi_uintn_t completion),
> >       u16 **abort_reason)
> >  {
> > +     bool updated;
> >       efi_status_t status;
> > +     struct fmp_state state = { 0 };
> >
> >       EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
> >                 image_size, vendor_code, progress, abort_reason);
> > @@ -338,14 +469,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
> >       if (!image || image_index != 1)
> >               return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > -     status = efi_firmware_capsule_authenticate(&image, &image_size);
> > +     status = efi_firmware_verify_image(&image, &image_size, image_index,
> > +                                        &state);
> >       if (status != EFI_SUCCESS)
> > -             return EFI_EXIT(status);
> > +             goto err;
> >
> > -     if (fit_update(image))
> > -             return EFI_EXIT(EFI_DEVICE_ERROR);
> > +     if (fit_update(image)) {
> > +             status = EFI_DEVICE_ERROR;
>
> I think we need to add here:
>                 state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;

Yes, I will fix it.

Thanks,
Masahisa Kojima

>
> -Takahiro Akashi
>
> > +             goto err;
> > +     }
> >
> > -     return EFI_EXIT(EFI_SUCCESS);
> > +     state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > +err:
> > +     updated = (status == EFI_SUCCESS) ? true : false;
> > +     efi_firmware_set_fmp_state_var(&state, image_index, updated);
> > +
> > +     return EFI_EXIT(status);
> >  }
> >
> >  const struct efi_firmware_management_protocol efi_fmp_fit = {
> > @@ -391,7 +530,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >       u16 **abort_reason)
> >  {
> >       int ret;
> > +     bool updated;
> >       efi_status_t status;
> > +     struct fmp_state state = { 0 };
> >
> >       EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
> >                 image_size, vendor_code, progress, abort_reason);
> > @@ -399,9 +540,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >       if (!image)
> >               return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > -     status = efi_firmware_capsule_authenticate(&image, &image_size);
> > +     status = efi_firmware_verify_image(&image, &image_size, image_index,
> > +                                        &state);
> >       if (status != EFI_SUCCESS)
> > -             return EFI_EXIT(status);
> > +             goto err;
> >
> >       if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> >               /*
> > @@ -410,16 +552,26 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >                */
> >               ret = fwu_get_image_index(&image_index);
> >               if (ret) {
> > +                     state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> >                       log_debug("Unable to get FWU image_index\n");
> > -                     return EFI_EXIT(EFI_DEVICE_ERROR);
> > +                     status = EFI_DEVICE_ERROR;
> > +                     goto err;
> >               }
> >       }
> >
> >       if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
> > -                          NULL, NULL))
> > -             return EFI_EXIT(EFI_DEVICE_ERROR);
> > +                          NULL, NULL)) {
> > +             status = EFI_DEVICE_ERROR;
> > +             state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> > +             goto err;
> > +     }
> > +
> > +     state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > +err:
> > +     updated = (status == EFI_SUCCESS) ? true : false;
> > +     efi_firmware_set_fmp_state_var(&state, image_index, updated);
> >
> > -     return EFI_EXIT(EFI_SUCCESS);
> > +     return EFI_EXIT(status);
> >  }
> >
> >  const struct efi_firmware_management_protocol efi_fmp_raw = {
> > --
> > 2.17.1
> >

On Wed, Mar 01, 2023 at 06:15:19PM +0900, Masahisa Kojima wrote:
> Firmware version management is not implemented in the current
> FMP protocol.
> EDK2 reference implementation capsule generation script inserts
> the FMP Payload Header right before the payload, it contains the
> firmware version and lowest supported version.
> 
> This commit utilizes the FMP Payload Header, read the header and
> stores the firmware version, lowest supported version,
> last attempt version and last attempt status into "FmpStateXXXX"
> EFI non-volatile variable. XXXX indicates the image index,
> since FMP protocol handles multiple image indexes.
> 
> This change is compatible with the existing FMP implementation.
> This change does not mandate the FMP Payload Header.
> If no FMP Payload Header is found in the capsule file, fw_version,
> lowest supported version, last attempt version and last attempt
> status is 0 and this is the same behavior as existing FMP
> implementation.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v2:
> - modify indent
> 
>  lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
>  1 file changed, 175 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 93e2b01c07..d1afafb052 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -10,6 +10,7 @@
>  #include <charset.h>
>  #include <dfu.h>
>  #include <efi_loader.h>
> +#include <efi_variable.h>
>  #include <fwu.h>
>  #include <image.h>
>  #include <signatures.h>
> @@ -18,6 +19,12 @@
>  
>  #define FMP_PAYLOAD_HDR_SIGNATURE	SIGNATURE_32('M', 'S', 'S', '1')
>  
> +#define EFI_FMP_STATE_GUID \
> +	EFI_GUID(0x84bed885, 0x193a, 0x403f, 0xa2, 0x78, \
> +		 0xe8, 0x9e, 0x23, 0x8a, 0xd6, 0xe1)
> +
> +static const efi_guid_t efi_guid_fmp_state = EFI_FMP_STATE_GUID;
> +
>  /**
>   * struct fmp_payload_header - EDK2 header for the FMP payload
>   *
> @@ -36,6 +43,24 @@ struct fmp_payload_header {
>  	u32 lowest_supported_version;
>  };
>  
> +/**
> + * struct fmp_state - fmp firmware update state
> + *
> + * This structure describes the state of the firmware update
> + * through FMP protocol.
> + *
> + * @fw_version:			Firmware versions used
> + * @lowest_supported_version:	Lowest supported version
> + * @last_attempt_version:	Last attempt version
> + * @last_attempt_status:	Last attempt status
> + */
> +struct fmp_state {
> +	u32 fw_version;
> +	u32 lowest_supported_version;
> +	u32 last_attempt_version;
> +	u32 last_attempt_status;
> +};
> +
>  __weak void set_dfu_alt_info(char *interface, char *devstr)
>  {
>  	env_set("dfu_alt_info", update_info.dfu_string);
> @@ -182,6 +207,7 @@ static efi_status_t efi_fill_image_desc_array(
>   * efi_firmware_capsule_authenticate - authenticate the capsule if enabled
>   * @p_image:		Pointer to new image
>   * @p_image_size:	Pointer to size of new image
> + * @state		Pointer to fmp state
>   *
>   * Authenticate the capsule if authentication is enabled.
>   * The image pointer and the image size are updated in case of success.
> @@ -190,12 +216,11 @@ static efi_status_t efi_fill_image_desc_array(
>   */
>  static
>  efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> -					       efi_uintn_t *p_image_size)
> +					       efi_uintn_t *p_image_size,
> +					       struct fmp_state *state)
>  {
>  	const void *image = *p_image;
>  	efi_uintn_t image_size = *p_image_size;
> -	u32 fmp_hdr_signature;
> -	struct fmp_payload_header *header;
>  	void *capsule_payload;
>  	efi_status_t status;
>  	efi_uintn_t capsule_payload_size;
> @@ -209,8 +234,12 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
>  
>  		if (status == EFI_SECURITY_VIOLATION) {
>  			printf("Capsule authentication check failed. Aborting update\n");
> +			state->last_attempt_status =
> +				LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
>  			return status;
>  		} else if (status != EFI_SUCCESS) {
> +			state->last_attempt_status =
> +				LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
>  			return status;
>  		}
>  
> @@ -222,24 +251,124 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
>  		debug("Updating capsule without authenticating.\n");
>  	}
>  
> -	fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> -	header = (void *)image;
> +	*p_image = image;
> +	*p_image_size = image_size;
> +
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_firmware_set_fmp_state_var - set FmpStateXXXX variable
> + * @state:		Pointer to fmp state
> + * @image_index:	image index
> + * @updated:		flag to indicate firmware update is successful
> + *
> + * Update the FmpStateXXXX variable with the firmware update state.
> + *
> + * Return:		status code
> + */
> +static
> +efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index,
> +					    bool updated)
> +{
> +	u16 varname[13]; /* u"FmpStateXXXX" */
> +	efi_status_t ret;
> +	efi_uintn_t size;
> +	struct fmp_state var_state = { 0 };
> +
> +	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> +				image_index);
> +	size = sizeof(var_state);
> +	ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
> +				   &var_state, NULL);
> +	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> +		return ret;
> +
> +	/*
> +	 * When the capsule update is successful, FmpStateXXXX variable is set
> +	 * according to the fmp payload header information. If there is no fmp payload
> +	 * header in the capsule file, all values are set to 0.
> +	 * When the capsule update fails, only last attempt information of FmpStateXXXX
> +	 * variable is updated, fw_version and lowest_supported_version keep original
> +	 * value or 0(in case no FmpStateXXXX variable found).
> +	 */
> +	if (updated) {
> +		var_state.fw_version = state->fw_version;
> +		var_state.lowest_supported_version = state->lowest_supported_version;
> +		var_state.last_attempt_version = state->last_attempt_version;
> +		var_state.last_attempt_status = state->last_attempt_status;
> +	} else {
> +		var_state.last_attempt_version = state->last_attempt_version;
> +		var_state.last_attempt_status = state->last_attempt_status;
> +	}
> +
> +	ret = efi_set_variable_int(varname, &efi_guid_fmp_state,
> +				   EFI_VARIABLE_NON_VOLATILE |
> +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				   EFI_VARIABLE_RUNTIME_ACCESS,
> +				   sizeof(var_state), &var_state, false);

One of my concerns about using UEFI variables as a storage for internal data
like firmwware status is that they are unnecessarily accessible to users.

In this sense, I think that those variable should be *read-only* and
maintained only by the system.

-Takahiro Akashi


> +	return ret;
> +}
>  
> +/**
> + * efi_firmware_parse_payload_header - parse FMP payload header
> + * @p_image:		Pointer to new image
> + * @p_image_size:	Pointer to size of new image
> + * @state		Pointer to fmp state
> + *
> + * Parse the FMP payload header and fill the fmp_state structure.
> + * If no FMP payload header is found, fmp_state structure is not updated.
> + *
> + */
> +static
> +void efi_firmware_parse_payload_header(const void **p_image,
> +				       efi_uintn_t *p_image_size,
> +				       struct fmp_state *state)
> +{
> +	const void *image = *p_image;
> +	efi_uintn_t image_size = *p_image_size;
> +	const struct fmp_payload_header *header;
> +	u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> +
> +	header = image;
>  	if (!memcmp(&header->signature, &fmp_hdr_signature,
>  		    sizeof(fmp_hdr_signature))) {
> -		/*
> -		 * When building the capsule with the scripts in
> -		 * edk2, a FMP header is inserted above the capsule
> -		 * payload. Compensate for this header to get the
> -		 * actual payload that is to be updated.
> -		 */
> +		/* FMP header is inserted above the capsule payload */
> +		state->fw_version = header->fw_version;
> +		state->lowest_supported_version = header->lowest_supported_version;
> +		state->last_attempt_version = header->fw_version;
>  		image += header->header_size;
>  		image_size -= header->header_size;
>  	}
>  
>  	*p_image = image;
>  	*p_image_size = image_size;
> -	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_firmware_verify_image - verify image
> + * @p_image:		Pointer to new image
> + * @p_image_size:	Pointer to size of new image
> + * @image_index		Image index
> + * @state		Pointer to fmp state
> + *
> + * Verify the capsule file
> + *
> + * Return:		status code
> + */
> +static
> +efi_status_t efi_firmware_verify_image(const void **p_image,
> +				       efi_uintn_t *p_image_size,
> +				       u8 image_index,
> +				       struct fmp_state *state)
> +{
> +	efi_status_t ret;
> +
> +	ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> +	efi_firmware_parse_payload_header(p_image, p_image_size, state);
> +
> +	return ret;
>  }
>  
>  /**
> @@ -330,7 +459,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	efi_status_t (*progress)(efi_uintn_t completion),
>  	u16 **abort_reason)
>  {
> +	bool updated;
>  	efi_status_t status;
> +	struct fmp_state state = { 0 };
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -338,14 +469,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	if (!image || image_index != 1)
>  		return EFI_EXIT(EFI_INVALID_PARAMETER);
>  
> -	status = efi_firmware_capsule_authenticate(&image, &image_size);
> +	status = efi_firmware_verify_image(&image, &image_size, image_index,
> +					   &state);
>  	if (status != EFI_SUCCESS)
> -		return EFI_EXIT(status);
> +		goto err;
>  
> -	if (fit_update(image))
> -		return EFI_EXIT(EFI_DEVICE_ERROR);
> +	if (fit_update(image)) {
> +		status = EFI_DEVICE_ERROR;
> +		goto err;
> +	}
>  
> -	return EFI_EXIT(EFI_SUCCESS);
> +	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +err:
> +	updated = (status == EFI_SUCCESS) ? true : false;
> +	efi_firmware_set_fmp_state_var(&state, image_index, updated);
> +
> +	return EFI_EXIT(status);
>  }
>  
>  const struct efi_firmware_management_protocol efi_fmp_fit = {
> @@ -391,7 +530,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  	u16 **abort_reason)
>  {
>  	int ret;
> +	bool updated;
>  	efi_status_t status;
> +	struct fmp_state state = { 0 };
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -399,9 +540,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  	if (!image)
>  		return EFI_EXIT(EFI_INVALID_PARAMETER);
>  
> -	status = efi_firmware_capsule_authenticate(&image, &image_size);
> +	status = efi_firmware_verify_image(&image, &image_size, image_index,
> +					   &state);
>  	if (status != EFI_SUCCESS)
> -		return EFI_EXIT(status);
> +		goto err;
>  
>  	if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
>  		/*
> @@ -410,16 +552,26 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  		 */
>  		ret = fwu_get_image_index(&image_index);
>  		if (ret) {
> +			state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
>  			log_debug("Unable to get FWU image_index\n");
> -			return EFI_EXIT(EFI_DEVICE_ERROR);
> +			status = EFI_DEVICE_ERROR;
> +			goto err;
>  		}
>  	}
>  
>  	if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
> -			     NULL, NULL))
> -		return EFI_EXIT(EFI_DEVICE_ERROR);
> +			     NULL, NULL)) {
> +		status = EFI_DEVICE_ERROR;
> +		state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> +		goto err;
> +	}
> +
> +	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +err:
> +	updated = (status == EFI_SUCCESS) ? true : false;
> +	efi_firmware_set_fmp_state_var(&state, image_index, updated);
>  
> -	return EFI_EXIT(EFI_SUCCESS);
> +	return EFI_EXIT(status);
>  }
>  
>  const struct efi_firmware_management_protocol efi_fmp_raw = {
> -- 
> 2.17.1
>

On Fri, 3 Mar 2023 at 09:17, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> On Wed, Mar 01, 2023 at 06:15:19PM +0900, Masahisa Kojima wrote:
> > Firmware version management is not implemented in the current
> > FMP protocol.
> > EDK2 reference implementation capsule generation script inserts
> > the FMP Payload Header right before the payload, it contains the
> > firmware version and lowest supported version.
> >
> > This commit utilizes the FMP Payload Header, read the header and
> > stores the firmware version, lowest supported version,
> > last attempt version and last attempt status into "FmpStateXXXX"
> > EFI non-volatile variable. XXXX indicates the image index,
> > since FMP protocol handles multiple image indexes.
> >
> > This change is compatible with the existing FMP implementation.
> > This change does not mandate the FMP Payload Header.
> > If no FMP Payload Header is found in the capsule file, fw_version,
> > lowest supported version, last attempt version and last attempt
> > status is 0 and this is the same behavior as existing FMP
> > implementation.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v2:
> > - modify indent
> >
> >  lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
> >  1 file changed, 175 insertions(+), 23 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > index 93e2b01c07..d1afafb052 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -10,6 +10,7 @@
> >  #include <charset.h>
> >  #include <dfu.h>
> >  #include <efi_loader.h>
> > +#include <efi_variable.h>
> >  #include <fwu.h>
> >  #include <image.h>
> >  #include <signatures.h>
> > @@ -18,6 +19,12 @@
> >
> >  #define FMP_PAYLOAD_HDR_SIGNATURE    SIGNATURE_32('M', 'S', 'S', '1')
> >
> > +#define EFI_FMP_STATE_GUID \
> > +     EFI_GUID(0x84bed885, 0x193a, 0x403f, 0xa2, 0x78, \
> > +              0xe8, 0x9e, 0x23, 0x8a, 0xd6, 0xe1)
> > +
> > +static const efi_guid_t efi_guid_fmp_state = EFI_FMP_STATE_GUID;
> > +
> >  /**
> >   * struct fmp_payload_header - EDK2 header for the FMP payload
> >   *
> > @@ -36,6 +43,24 @@ struct fmp_payload_header {
> >       u32 lowest_supported_version;
> >  };
> >
> > +/**
> > + * struct fmp_state - fmp firmware update state
> > + *
> > + * This structure describes the state of the firmware update
> > + * through FMP protocol.
> > + *
> > + * @fw_version:                      Firmware versions used
> > + * @lowest_supported_version:        Lowest supported version
> > + * @last_attempt_version:    Last attempt version
> > + * @last_attempt_status:     Last attempt status
> > + */
> > +struct fmp_state {
> > +     u32 fw_version;
> > +     u32 lowest_supported_version;
> > +     u32 last_attempt_version;
> > +     u32 last_attempt_status;
> > +};
> > +
> >  __weak void set_dfu_alt_info(char *interface, char *devstr)
> >  {
> >       env_set("dfu_alt_info", update_info.dfu_string);
> > @@ -182,6 +207,7 @@ static efi_status_t efi_fill_image_desc_array(
> >   * efi_firmware_capsule_authenticate - authenticate the capsule if enabled
> >   * @p_image:         Pointer to new image
> >   * @p_image_size:    Pointer to size of new image
> > + * @state            Pointer to fmp state
> >   *
> >   * Authenticate the capsule if authentication is enabled.
> >   * The image pointer and the image size are updated in case of success.
> > @@ -190,12 +216,11 @@ static efi_status_t efi_fill_image_desc_array(
> >   */
> >  static
> >  efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> > -                                            efi_uintn_t *p_image_size)
> > +                                            efi_uintn_t *p_image_size,
> > +                                            struct fmp_state *state)
> >  {
> >       const void *image = *p_image;
> >       efi_uintn_t image_size = *p_image_size;
> > -     u32 fmp_hdr_signature;
> > -     struct fmp_payload_header *header;
> >       void *capsule_payload;
> >       efi_status_t status;
> >       efi_uintn_t capsule_payload_size;
> > @@ -209,8 +234,12 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> >
> >               if (status == EFI_SECURITY_VIOLATION) {
> >                       printf("Capsule authentication check failed. Aborting update\n");
> > +                     state->last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
> >                       return status;
> >               } else if (status != EFI_SUCCESS) {
> > +                     state->last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> >                       return status;
> >               }
> >
> > @@ -222,24 +251,124 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> >               debug("Updating capsule without authenticating.\n");
> >       }
> >
> > -     fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> > -     header = (void *)image;
> > +     *p_image = image;
> > +     *p_image_size = image_size;
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * efi_firmware_set_fmp_state_var - set FmpStateXXXX variable
> > + * @state:           Pointer to fmp state
> > + * @image_index:     image index
> > + * @updated:         flag to indicate firmware update is successful
> > + *
> > + * Update the FmpStateXXXX variable with the firmware update state.
> > + *
> > + * Return:           status code
> > + */
> > +static
> > +efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index,
> > +                                         bool updated)
> > +{
> > +     u16 varname[13]; /* u"FmpStateXXXX" */
> > +     efi_status_t ret;
> > +     efi_uintn_t size;
> > +     struct fmp_state var_state = { 0 };
> > +
> > +     efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > +                             image_index);
> > +     size = sizeof(var_state);
> > +     ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
> > +                                &var_state, NULL);
> > +     if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> > +             return ret;
> > +
> > +     /*
> > +      * When the capsule update is successful, FmpStateXXXX variable is set
> > +      * according to the fmp payload header information. If there is no fmp payload
> > +      * header in the capsule file, all values are set to 0.
> > +      * When the capsule update fails, only last attempt information of FmpStateXXXX
> > +      * variable is updated, fw_version and lowest_supported_version keep original
> > +      * value or 0(in case no FmpStateXXXX variable found).
> > +      */
> > +     if (updated) {
> > +             var_state.fw_version = state->fw_version;
> > +             var_state.lowest_supported_version = state->lowest_supported_version;
> > +             var_state.last_attempt_version = state->last_attempt_version;
> > +             var_state.last_attempt_status = state->last_attempt_status;
> > +     } else {
> > +             var_state.last_attempt_version = state->last_attempt_version;
> > +             var_state.last_attempt_status = state->last_attempt_status;
> > +     }
> > +
> > +     ret = efi_set_variable_int(varname, &efi_guid_fmp_state,
> > +                                EFI_VARIABLE_NON_VOLATILE |
> > +                                EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                EFI_VARIABLE_RUNTIME_ACCESS,
> > +                                sizeof(var_state), &var_state, false);
>
> One of my concerns about using UEFI variables as a storage for internal data
> like firmwware status is that they are unnecessarily accessible to users.
>
> In this sense, I think that those variable should be *read-only* and
> maintained only by the system.

Yes, this FmpState variable should be read-only.

Thanks,
Masahisa Kojima

>
> -Takahiro Akashi
>
>
> > +     return ret;
> > +}
> >
> > +/**
> > + * efi_firmware_parse_payload_header - parse FMP payload header
> > + * @p_image:         Pointer to new image
> > + * @p_image_size:    Pointer to size of new image
> > + * @state            Pointer to fmp state
> > + *
> > + * Parse the FMP payload header and fill the fmp_state structure.
> > + * If no FMP payload header is found, fmp_state structure is not updated.
> > + *
> > + */
> > +static
> > +void efi_firmware_parse_payload_header(const void **p_image,
> > +                                    efi_uintn_t *p_image_size,
> > +                                    struct fmp_state *state)
> > +{
> > +     const void *image = *p_image;
> > +     efi_uintn_t image_size = *p_image_size;
> > +     const struct fmp_payload_header *header;
> > +     u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> > +
> > +     header = image;
> >       if (!memcmp(&header->signature, &fmp_hdr_signature,
> >                   sizeof(fmp_hdr_signature))) {
> > -             /*
> > -              * When building the capsule with the scripts in
> > -              * edk2, a FMP header is inserted above the capsule
> > -              * payload. Compensate for this header to get the
> > -              * actual payload that is to be updated.
> > -              */
> > +             /* FMP header is inserted above the capsule payload */
> > +             state->fw_version = header->fw_version;
> > +             state->lowest_supported_version = header->lowest_supported_version;
> > +             state->last_attempt_version = header->fw_version;
> >               image += header->header_size;
> >               image_size -= header->header_size;
> >       }
> >
> >       *p_image = image;
> >       *p_image_size = image_size;
> > -     return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * efi_firmware_verify_image - verify image
> > + * @p_image:         Pointer to new image
> > + * @p_image_size:    Pointer to size of new image
> > + * @image_index              Image index
> > + * @state            Pointer to fmp state
> > + *
> > + * Verify the capsule file
> > + *
> > + * Return:           status code
> > + */
> > +static
> > +efi_status_t efi_firmware_verify_image(const void **p_image,
> > +                                    efi_uintn_t *p_image_size,
> > +                                    u8 image_index,
> > +                                    struct fmp_state *state)
> > +{
> > +     efi_status_t ret;
> > +
> > +     ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> > +     efi_firmware_parse_payload_header(p_image, p_image_size, state);
> > +
> > +     return ret;
> >  }
> >
> >  /**
> > @@ -330,7 +459,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
> >       efi_status_t (*progress)(efi_uintn_t completion),
> >       u16 **abort_reason)
> >  {
> > +     bool updated;
> >       efi_status_t status;
> > +     struct fmp_state state = { 0 };
> >
> >       EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
> >                 image_size, vendor_code, progress, abort_reason);
> > @@ -338,14 +469,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
> >       if (!image || image_index != 1)
> >               return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > -     status = efi_firmware_capsule_authenticate(&image, &image_size);
> > +     status = efi_firmware_verify_image(&image, &image_size, image_index,
> > +                                        &state);
> >       if (status != EFI_SUCCESS)
> > -             return EFI_EXIT(status);
> > +             goto err;
> >
> > -     if (fit_update(image))
> > -             return EFI_EXIT(EFI_DEVICE_ERROR);
> > +     if (fit_update(image)) {
> > +             status = EFI_DEVICE_ERROR;
> > +             goto err;
> > +     }
> >
> > -     return EFI_EXIT(EFI_SUCCESS);
> > +     state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > +err:
> > +     updated = (status == EFI_SUCCESS) ? true : false;
> > +     efi_firmware_set_fmp_state_var(&state, image_index, updated);
> > +
> > +     return EFI_EXIT(status);
> >  }
> >
> >  const struct efi_firmware_management_protocol efi_fmp_fit = {
> > @@ -391,7 +530,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >       u16 **abort_reason)
> >  {
> >       int ret;
> > +     bool updated;
> >       efi_status_t status;
> > +     struct fmp_state state = { 0 };
> >
> >       EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
> >                 image_size, vendor_code, progress, abort_reason);
> > @@ -399,9 +540,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >       if (!image)
> >               return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > -     status = efi_firmware_capsule_authenticate(&image, &image_size);
> > +     status = efi_firmware_verify_image(&image, &image_size, image_index,
> > +                                        &state);
> >       if (status != EFI_SUCCESS)
> > -             return EFI_EXIT(status);
> > +             goto err;
> >
> >       if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> >               /*
> > @@ -410,16 +552,26 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >                */
> >               ret = fwu_get_image_index(&image_index);
> >               if (ret) {
> > +                     state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> >                       log_debug("Unable to get FWU image_index\n");
> > -                     return EFI_EXIT(EFI_DEVICE_ERROR);
> > +                     status = EFI_DEVICE_ERROR;
> > +                     goto err;
> >               }
> >       }
> >
> >       if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
> > -                          NULL, NULL))
> > -             return EFI_EXIT(EFI_DEVICE_ERROR);
> > +                          NULL, NULL)) {
> > +             status = EFI_DEVICE_ERROR;
> > +             state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> > +             goto err;
> > +     }
> > +
> > +     state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > +err:
> > +     updated = (status == EFI_SUCCESS) ? true : false;
> > +     efi_firmware_set_fmp_state_var(&state, image_index, updated);
> >
> > -     return EFI_EXIT(EFI_SUCCESS);
> > +     return EFI_EXIT(status);
> >  }
> >
> >  const struct efi_firmware_management_protocol efi_fmp_raw = {
> > --
> > 2.17.1
> >