[PATCH 2/2] eficonfig: avoid SetVariable between GetNextVariableName calls

[Heinrich Schuchardt]

On 12/8/22 05:40, Masahisa Kojima wrote:
> The current code calls efi_set_variable_int() to delete the
> invalid boot option between calls to efi_get_next_variable_name_int(),
> it may produce unpredictable results.
>
> This commit moves removal of the invalid boot option outside
> of the efi_get_next_variable_name_int() calls.
> EFI_NOT_FOUND returned from efi_get_next_variable_name_int()
> indicates we retrieve all EFI variables, it should be treated

Thanks for the correction.

%s/retrieve/retrieved/

> as EFI_SUCEESS.
>
> To address the checkpatch warning of too many leading tabs,
> combine two if statement into one.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> ---
>   cmd/eficonfig.c | 59 +++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 45 insertions(+), 14 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index cd7a51cc7e..e967d8da4d 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -2310,13 +2310,14 @@ out:
>   efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt,
>   						  efi_status_t count)
>   {
> -	u32 i;
>   	efi_uintn_t size;
>   	void *load_option;
> +	u32 i, list_size = 0;
>   	struct efi_load_option lo;
>   	u16 *var_name16 = NULL;
>   	u16 varname[] = u"Boot####";
>   	efi_status_t ret = EFI_SUCCESS;
> +	u16 *delete_index_list = NULL, *p;
>   	efi_uintn_t buf_size;
>
>   	buf_size = 128;
> @@ -2331,8 +2332,14 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
>   		efi_uintn_t tmp;
>
>   		ret = efi_get_variable_name(&buf_size, &var_name16, &guid);
> -		if (ret == EFI_NOT_FOUND)
> +		if (ret == EFI_NOT_FOUND) {
> +			/*
> +			 * EFI_NOT_FOUND indicates we retrieve all EFI variables,

%s/retrieve/retrieved/

> +			 * should be treated as success.

This should be ...

> +			 */
> +			ret = EFI_SUCCESS;
>   			break;
> +		}
>   		if (ret != EFI_SUCCESS)
>   			goto out;
>
> @@ -2349,31 +2356,55 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
>   		if (ret != EFI_SUCCESS)
>   			goto next;
>
> -		if (size >= sizeof(efi_guid_bootmenu_auto_generated)) {
> -			if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) {
> -				for (i = 0; i < count; i++) {
> -					if (opt[i].size == tmp &&
> -					    memcmp(opt[i].lo, load_option, tmp) == 0) {
> -						opt[i].exist = true;
> -						break;
> -					}
> +		if (size >= sizeof(efi_guid_bootmenu_auto_generated) &&
> +		    guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) {

According to the U-Boot programming style we prefer '!guidcmp()' over
'guidcmp() == 0'.

> +			for (i = 0; i < count; i++) {
> +				if (opt[i].size == tmp &&
> +				    memcmp(opt[i].lo, load_option, tmp) == 0) {
> +					opt[i].exist = true;
> +					break;
>   				}
> +			}
>
> -				if (i == count) {
> -					ret = delete_boot_option(i);
> -					if (ret != EFI_SUCCESS) {
> -						free(load_option);
> +			/*
> +			 * The entire list of variables must be retrieved by
> +			 * efi_get_next_variable_name_int() before deleting the invalid
> +			 * boot option, just save the index here.
> +			 */
> +			if (i == count) {
> +				if (!delete_index_list) {
> +					delete_index_list = malloc(sizeof(u32));

The malloc branch is superfluous. Realloc called with NULL does a malloc.

Best regards

Heinrich

> +					if (!delete_index_list) {
> +						ret = EFI_OUT_OF_RESOURCES;
>   						goto out;
>   					}
> +				} else {
> +					p = realloc(delete_index_list, sizeof(u32) *
> +						    (list_size + 1));
> +					if (!p) {
> +						ret = EFI_OUT_OF_RESOURCES;
> +						goto out;
> +					}
> +					delete_index_list = p;
>   				}
> +
> +				delete_index_list[list_size++] = index;
>   			}
>   		}
>   next:
>   		free(load_option);
>   	}
>
> +	/* delete all invalid boot options */
> +	for (i = 0; i < list_size; i++) {
> +		ret = delete_boot_option(delete_index_list[i]);
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +	}
> +
>   out:
>   	free(var_name16);
> +	free(delete_index_list);
>
>   	return ret;
>   }