Re: [PATCH v6 5/8] efi_loader: check lowest supported version

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

On Fri, May 19, 2023 at 07:32:11PM +0900, Masahisa Kojima wrote:
> The FMP Payload Header which EDK II capsule generation scripts
> insert has a firmware version.
> This commit reads the lowest supported version stored in the
> device tree, then check if the firmware version in FMP payload header
> of the ongoing capsule is equal or greater than the
> lowest supported version. If the firmware version is lower than
> lowest supported version, capsule update will not be performed.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v6:
> - get aligned to the latest implementation
>
> Changes in v5:
> - newly implement the device tree based versioning
>
> Changes in v4:
> - use log_err() instead of printf()
>
> Changes in v2:
> - add error message when the firmware version is lower than
>   lowest supported version
>
>  lib/efi_loader/efi_firmware.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 00cf9a088a..7cd0016765 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -424,7 +424,8 @@ static void efi_firmware_get_fw_version(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
>   */
> @@ -434,11 +435,27 @@ efi_status_t efi_firmware_verify_image(const void **p_image,
>  				       u8 image_index,
>  				       struct fmp_state *state)
>  {
> +	u32 lsv;
>  	efi_status_t ret;
> +	efi_guid_t *image_type_id;
>
>  	ret = efi_firmware_capsule_authenticate(p_image, p_image_size);
>  	efi_firmware_get_fw_version(p_image, p_image_size, state);
>
> +	/* check lowest_supported_version if capsule authentication passes */
> +	if (ret == EFI_SUCCESS) {

What's the point of this here?  Can;'t we move this check right after
efi_firmware_capsule_authenticate() and return a security violation if that
failed?

> +		image_type_id = efi_firmware_get_image_type_id(image_index);
> +		if (!image_type_id)
> +			return EFI_INVALID_PARAMETER;
> +
> +		efi_firmware_get_lsv_from_dtb(image_index, image_type_id, &lsv);
> +		if (state->fw_version < lsv) {
> +			log_err("Firmware version %u too low. Expecting >= %u. Aborting update\n",
> +				state->fw_version, lsv);
> +			return EFI_INVALID_PARAMETER;
> +		}
> +	}
> +
>  	return ret;
>  }
>
> --
> 2.17.1
>

Thanks
/Ilias