Re: [PATCH v6 2/8] efi_loader: store firmware version into FmpState variable

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Kojima-san,

On Fri, May 19, 2023 at 07:32:08PM +0900, Masahisa Kojima wrote:
> Firmware version management is not implemented in the current
> FMP protocol.
> EDK II reference implementation capsule generation script inserts
> the FMP Payload Header right before the payload, FMP Payload Header
> contains the firmware version and lowest supported version.
>
> This commit utilizes the FMP Payload Header, reads the header and
> stores the firmware version into "FmpStateXXXX" EFI non-volatile variable.
> XXXX indicates the image index, since FMP protocol handles multiple
> image indexes.
> Note that lowest supported version included in the FMP Payload Header
> is not used. If the platform uses file-based EFI variable storage,
> it can be tampered. The file-based EFI variable storage is not the
> right place to store the lowest supported version for anti-rollback
> protection.
>
> 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>
> ---
> Changed in v6:
> - only store the fw_version in the FmpState EFI variable
>
> Changes in v4:
> - move lines that are the same in both branches out of the if statement
> - s/EDK2/EDK II/
> - create print result function
> - set last_attempt_version when capsule authentication failed
> - use log_err() instead of printf()
>
> Changes in v3:
> - exclude CONFIG_FWU_MULTI_BANK_UPDATE case
> - set image_type_id as a vendor field of FmpStateXXXX variable
> - set READ_ONLY flag for FmpStateXXXX variable
> - add error code for FIT image case
>
> Changes in v2:
> - modify indent
>

[...]

> +/**
> + * efi_firmware_get_fw_version - get fw_version from 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_get_fw_version(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 (header->signature == fmp_hdr_signature) {
> +		/* FMP header is inserted above the capsule payload */
> +		state->fw_version = header->fw_version;
>
> -	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.
> -		 */
>  		image += header->header_size;
>  		image_size -= header->header_size;
>  	}

>
>  	*p_image = image;
>  	*p_image_size = image_size;

Can we get rid of the extra image/image_size here and move this inside the
if()?

> -	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_firmware_verify_image - verify image

The name is a bit generic here, we need something which describes what
happens better.  The verification already happens in
efi_firmware_capsule_authenticate().
Maybe efi_prepare_capsule() or something like that ?

> + * @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);
> +	efi_firmware_get_fw_version(p_image, p_image_size, state);
> +
> +	return ret;
>  }
>
>  /**
> @@ -331,6 +454,7 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	u16 **abort_reason)
>  {
>  	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,13 +462,16 @@ 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);
>
>  	if (fit_update(image))
>  		return EFI_EXIT(EFI_DEVICE_ERROR);
>
> +	efi_firmware_set_fmp_state_var(&state, image_index);
> +
>  	return EFI_EXIT(EFI_SUCCESS);
>  }
>
> @@ -392,6 +519,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  {
>  	int ret;
>  	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,7 +527,8 @@ 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);
>
> @@ -419,6 +548,8 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  			     NULL, NULL))
>  		return EFI_EXIT(EFI_DEVICE_ERROR);
>
> +	efi_firmware_set_fmp_state_var(&state, image_index);
> +
>  	return EFI_EXIT(EFI_SUCCESS);
>  }
>
> --
> 2.17.1
>

Thanks
/Ilias