Re: [PATCH v4 3/8] capsule: Put a check for image index before the update

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

On Thu, Mar 31, 2022 at 06:57:45PM +0530, Sughosh Ganu wrote:
> The current capsule update code compares the image GUID value in the
> capsule header with the image GUID value obtained from the
> GetImageInfo function of the Firmware Management Protocol(FMP). This
> comparison is done to ascertain if the FMP's SetImage function can be
> called for the update. Make this checking more robust by comparing the
> image_index value passed through the capsule with that returned by the
> FMP's GetImageInfo function. This protects against the scenario of the
> firmware being updated in a wrong partition/location on the storage
> device if an incorrect value has been passed through the capsule,
> since the image_index is used to determine the location of the update
> on the storage device.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
> 
> Changes since V3: None
> 
>  lib/efi_loader/efi_capsule.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index f00440163d..f03f4c9044 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -128,6 +128,7 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule,
>  /**
>   * efi_fmp_find - search for Firmware Management Protocol drivers
>   * @image_type:		Image type guid
> + * @image_index:	Image Index
>   * @instance:		Instance number
>   * @handles:		Handles of FMP drivers
>   * @no_handles:		Number of handles
> @@ -141,8 +142,8 @@ void set_capsule_result(int index, struct efi_capsule_header *capsule,
>   * * NULL		- on failure
>   */
>  static struct efi_firmware_management_protocol *
> -efi_fmp_find(efi_guid_t *image_type, u64 instance, efi_handle_t *handles,
> -	     efi_uintn_t no_handles)
> +efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
> +	     efi_handle_t *handles, efi_uintn_t no_handles)
>  {
>  	efi_handle_t *handle;
>  	struct efi_firmware_management_protocol *fmp;
> @@ -203,6 +204,7 @@ efi_fmp_find(efi_guid_t *image_type, u64 instance, efi_handle_t *handles,
>  			log_debug("+++ desc[%d] index: %d, name: %ls\n",
>  				  j, desc->image_index, desc->image_id_name);
>  			if (!guidcmp(&desc->image_type_id, image_type) &&
> +			    (desc->image_index == image_index) &&
>  			    (!instance ||
>  			     !desc->hardware_instance ||
>  			      desc->hardware_instance == instance))
> @@ -449,8 +451,8 @@ static efi_status_t efi_capsule_update_firmware(
>  		}
>  
>  		/* find a device for update firmware */
> -		/* TODO: should we pass index as well, or nothing but type? */
>  		fmp = efi_fmp_find(&image->update_image_type_id,
> +				   image->update_image_index,
>  				   image->update_hardware_instance,
>  				   handles, no_handles);
>  		if (!fmp) {
> -- 
> 2.25.1
> 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>