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.koj...@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