On Tue, 23 May 2023 at 06:24, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > 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()?
Yes, thanks. > > > - 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(). In the "[PATCH v6 5/8] efi_loader: check lowest supported version" in this series, checking lowest_supported_version is added in efi_firmware_verify_image(). So, can we keep this function name? Regards, Masahisa Kojima > 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