diff mbox series

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

Message ID 20220331132750.1532722-4-sughosh.ganu@linaro.org
State Superseded
Headers show
Series efi: capsule: Capsule Update fixes and enhancements | expand

Commit Message

Sughosh Ganu March 31, 2022, 1:27 p.m. UTC 
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(-)

Comments

Ilias Apalodimas March 31, 2022, 6:47 p.m. UTC | #1 
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>
diff mbox series

Patch

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) {