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

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

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>