[PATCH v2] efi_loader: fix iteration of FMP protocols

[PATCH v2] efi_loader: fix iteration of FMP protocols

[Masahisa Kojima]

If one of the FMP protocols fails when calling GetImageInfo(),
populating the ESRT ends up with failure and other FMP protocols
are not added to the ESRT. We should still add all other FMP
protocols to the ESRT.

With this commit, iteration of all FMP protocols continues
even though one of the FMP protocols fails.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Note that this patch addresses the following issue.
https://source.denx.de/u-boot/custodians/u-boot-efi/-/issues/3

changes in v2
- add error handling when num_entries is 0 after calling fmp->get_image_info()

 lib/efi_loader/efi_esrt.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_esrt.c b/lib/efi_loader/efi_esrt.c
index 7f46d651e6..1f629a6023 100644
--- a/lib/efi_loader/efi_esrt.c
+++ b/lib/efi_loader/efi_esrt.c
@@ -365,7 +365,7 @@ efi_status_t efi_esrt_populate(void)
 		if (ret != EFI_SUCCESS) {
 			EFI_PRINT("ESRT Unable to find FMP handle (%u)\n",
 				  idx);
-			goto out;
+			continue;
 		}
 		fmp = handler->protocol_interface;
 
@@ -380,15 +380,14 @@ efi_status_t efi_esrt_populate(void)
 			 * fmp->get_image_info to return BUFFER_TO_SMALL.
 			 */
 			EFI_PRINT("ESRT erroneous FMP implementation\n");
-			ret = EFI_INVALID_PARAMETER;
-			goto out;
+			continue;
 		}
 
 		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, info_size,
 					(void **)&img_info);
 		if (ret != EFI_SUCCESS) {
 			EFI_PRINT("ESRT failed to allocate memory for image info\n");
-			goto out;
+			continue;
 		}
 
 		/*
@@ -406,7 +405,7 @@ efi_status_t efi_esrt_populate(void)
 		if (ret != EFI_SUCCESS) {
 			EFI_PRINT("ESRT failed to obtain image info from FMP\n");
 			efi_free_pool(img_info);
-			goto out;
+			continue;
 		}
 
 		num_entries += desc_count;
@@ -414,6 +413,13 @@ efi_status_t efi_esrt_populate(void)
 		efi_free_pool(img_info);
 	}
 
+	/* error occurs in fmp->get_image_info() if num_entries is 0 here */
+	if (!num_entries) {
+		EFI_PRINT("Error occurs, num_entries should not be 0\n");
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
 	EFI_PRINT("ESRT create table with %u entries\n", num_entries);
 	/*
 	 * Allocate an ESRT with the sufficient number of entries to accommodate
@@ -437,7 +443,7 @@ efi_status_t efi_esrt_populate(void)
 		if (ret != EFI_SUCCESS) {
 			EFI_PRINT("ESRT unable to find FMP handle (%u)\n",
 				  idx);
-			break;
+			continue;
 		}
 		fmp = handler->protocol_interface;
 
-- 
2.34.1

Re: [PATCH v2] efi_loader: fix iteration of FMP protocols

[Ilias Apalodimas]

On Mon, Dec 18, 2023 at 06:57:41PM +0900, Masahisa Kojima wrote:
> If one of the FMP protocols fails when calling GetImageInfo(),
> populating the ESRT ends up with failure and other FMP protocols
> are not added to the ESRT. We should still add all other FMP
> protocols to the ESRT.
>
> With this commit, iteration of all FMP protocols continues
> even though one of the FMP protocols fails.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Note that this patch addresses the following issue.
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/issues/3
>
> changes in v2
> - add error handling when num_entries is 0 after calling fmp->get_image_info()
>
>  lib/efi_loader/efi_esrt.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/lib/efi_loader/efi_esrt.c b/lib/efi_loader/efi_esrt.c
> index 7f46d651e6..1f629a6023 100644
> --- a/lib/efi_loader/efi_esrt.c
> +++ b/lib/efi_loader/efi_esrt.c
> @@ -365,7 +365,7 @@ efi_status_t efi_esrt_populate(void)
>  		if (ret != EFI_SUCCESS) {
>  			EFI_PRINT("ESRT Unable to find FMP handle (%u)\n",
>  				  idx);
> -			goto out;
> +			continue;
>  		}
>  		fmp = handler->protocol_interface;
>
> @@ -380,15 +380,14 @@ efi_status_t efi_esrt_populate(void)
>  			 * fmp->get_image_info to return BUFFER_TO_SMALL.
>  			 */
>  			EFI_PRINT("ESRT erroneous FMP implementation\n");
> -			ret = EFI_INVALID_PARAMETER;
> -			goto out;
> +			continue;
>  		}
>
>  		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, info_size,
>  					(void **)&img_info);
>  		if (ret != EFI_SUCCESS) {
>  			EFI_PRINT("ESRT failed to allocate memory for image info\n");
> -			goto out;
> +			continue;
>  		}
>
>  		/*
> @@ -406,7 +405,7 @@ efi_status_t efi_esrt_populate(void)
>  		if (ret != EFI_SUCCESS) {
>  			EFI_PRINT("ESRT failed to obtain image info from FMP\n");
>  			efi_free_pool(img_info);
> -			goto out;
> +			continue;
>  		}
>
>  		num_entries += desc_count;
> @@ -414,6 +413,13 @@ efi_status_t efi_esrt_populate(void)
>  		efi_free_pool(img_info);
>  	}
>
> +	/* error occurs in fmp->get_image_info() if num_entries is 0 here */
> +	if (!num_entries) {
> +		EFI_PRINT("Error occurs, num_entries should not be 0\n");
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
>  	EFI_PRINT("ESRT create table with %u entries\n", num_entries);
>  	/*
>  	 * Allocate an ESRT with the sufficient number of entries to accommodate
> @@ -437,7 +443,7 @@ efi_status_t efi_esrt_populate(void)
>  		if (ret != EFI_SUCCESS) {
>  			EFI_PRINT("ESRT unable to find FMP handle (%u)\n",
>  				  idx);
> -			break;
> +			continue;
>  		}
>  		fmp = handler->protocol_interface;
>
> --
> 2.34.1
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
For the SynQuacer
Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>