[PATCH v2 0/2] fix eficonfig GetNextVariableName calls handling

[PATCH v2 0/2] fix eficonfig GetNextVariableName calls handling

[Masahisa Kojima]

This series includes refactoring and bugfix of GetNextVariableName
calls in eficonfig.

After "eficonfig: carve out efi_get_next_variable_name_int calls" patch
is merged, I will send the follow-up patch to use common function in
cmd/efidebug.c and cmd/nvedit_efi.c. These files also implement
alloc -> efi_get_next_variable_name_int -> realloc ->
efi_get_next_variable_name_int sequence.

Masahisa Kojima (2):
  eficonfig: carve out efi_get_next_variable_name_int calls
  eficonfig: avoid SetVariable between GetNextVariableName calls

 cmd/eficonfig.c             | 114 ++++++++++++++++--------------------
 include/efi_loader.h        |   2 +
 lib/efi_loader/efi_helper.c |  34 +++++++++++
 3 files changed, 88 insertions(+), 62 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] eficonfig: carve out efi_get_next_variable_name_int calls

[Masahisa Kojima]

To retrieve the EFI variable name by efi_get_next_variable_name_int(),
the sequence of alloc -> efi_get_next_variable_name_int ->
realloc -> efi_get_next_variable_name_int is required.
In current code, this sequence repeatedly appears in
the several functions. It should be curved out a common function.

This commit also fixes the missing free() of var_name16
in eficonfig_delete_invalid_boot_option().

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v2:
- fix typo in the commit message
- rename efi_get_variable_name to efi_next_variable_name

 cmd/eficonfig.c             | 62 +++++++++----------------------------
 include/efi_loader.h        |  2 ++
 lib/efi_loader/efi_helper.c | 34 ++++++++++++++++++++
 3 files changed, 51 insertions(+), 47 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 394ae67cce..0b07dfc958 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -1683,7 +1683,7 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
 	u32 i;
 	u16 *bootorder;
 	efi_status_t ret;
-	u16 *var_name16 = NULL, *p;
+	u16 *var_name16 = NULL;
 	efi_uintn_t num, size, buf_size;
 	struct efimenu *efi_menu;
 	struct list_head *pos, *n;
@@ -1718,24 +1718,12 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
 		int index;
 		efi_guid_t guid;
 
-		size = buf_size;
-		ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
+		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
 		if (ret == EFI_NOT_FOUND)
 			break;
-		if (ret == EFI_BUFFER_TOO_SMALL) {
-			buf_size = size;
-			p = realloc(var_name16, buf_size);
-			if (!p) {
-				free(var_name16);
-				return EFI_OUT_OF_RESOURCES;
-			}
-			var_name16 = p;
-			ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
-		}
-		if (ret != EFI_SUCCESS) {
-			free(var_name16);
-			return ret;
-		}
+		if (ret != EFI_SUCCESS)
+			goto out;
+
 		if (efi_varname_is_load_option(var_name16, &index)) {
 			/* If the index is included in the BootOrder, skip it */
 			if (search_bootorder(bootorder, num, index, NULL))
@@ -2026,7 +2014,7 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
 	u32 i;
 	char *title;
 	efi_status_t ret;
-	u16 *var_name16 = NULL, *p;
+	u16 *var_name16 = NULL;
 	efi_uintn_t size, buf_size;
 
 	/* list the load option in the order of BootOrder variable */
@@ -2054,19 +2042,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
 			break;
 
 		size = buf_size;
-		ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
+		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
 		if (ret == EFI_NOT_FOUND)
 			break;
-		if (ret == EFI_BUFFER_TOO_SMALL) {
-			buf_size = size;
-			p = realloc(var_name16, buf_size);
-			if (!p) {
-				ret = EFI_OUT_OF_RESOURCES;
-				goto out;
-			}
-			var_name16 = p;
-			ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
-		}
 		if (ret != EFI_SUCCESS)
 			goto out;
 
@@ -2336,10 +2314,10 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
 	efi_uintn_t size;
 	void *load_option;
 	struct efi_load_option lo;
-	u16 *var_name16 = NULL, *p;
+	u16 *var_name16 = NULL;
 	u16 varname[] = u"Boot####";
 	efi_status_t ret = EFI_SUCCESS;
-	efi_uintn_t varname_size, buf_size;
+	efi_uintn_t buf_size;
 
 	buf_size = 128;
 	var_name16 = malloc(buf_size);
@@ -2352,24 +2330,12 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
 		efi_guid_t guid;
 		efi_uintn_t tmp;
 
-		varname_size = buf_size;
-		ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid);
+		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
 		if (ret == EFI_NOT_FOUND)
 			break;
-		if (ret == EFI_BUFFER_TOO_SMALL) {
-			buf_size = varname_size;
-			p = realloc(var_name16, buf_size);
-			if (!p) {
-				free(var_name16);
-				return EFI_OUT_OF_RESOURCES;
-			}
-			var_name16 = p;
-			ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid);
-		}
-		if (ret != EFI_SUCCESS) {
-			free(var_name16);
-			return ret;
-		}
+		if (ret != EFI_SUCCESS)
+			goto out;
+
 		if (!efi_varname_is_load_option(var_name16, &index))
 			continue;
 
@@ -2407,6 +2373,8 @@ next:
 	}
 
 out:
+	free(var_name16);
+
 	return ret;
 }
 
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0899e293e5..699176872d 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -708,6 +708,8 @@ int algo_to_len(const char *algo);
 int efi_link_dev(efi_handle_t handle, struct udevice *dev);
 int efi_unlink_dev(efi_handle_t handle);
 bool efi_varname_is_load_option(u16 *var_name16, int *index);
+efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
+				    efi_guid_t *guid);
 
 /**
  * efi_size_in_pages() - convert size in bytes to size in pages
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index 788cb9faec..1f4ab2b419 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -223,3 +223,37 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index)
 
 	return false;
 }
+
+/**
+ * efi_next_variable_name() - get next variable name
+ *
+ * This function is a wrapper of efi_get_next_variable_name_int().
+ * If efi_get_next_variable_name_int() returns EFI_BUFFER_TOO_SMALL,
+ * @size and @buf are updated by new buffer size and realloced buffer.
+ *
+ * @size:	pointer to the buffer size
+ * @buf:	pointer to the buffer
+ * @guid:	pointer to the guid
+ * Return:	status code
+ */
+efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, efi_guid_t *guid)
+{
+	u16 *p;
+	efi_status_t ret;
+	efi_uintn_t buf_size = *size;
+
+	ret = efi_get_next_variable_name_int(&buf_size, *buf, guid);
+	if (ret == EFI_NOT_FOUND)
+		return ret;
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		p = realloc(*buf, buf_size);
+		if (!p)
+			return EFI_OUT_OF_RESOURCES;
+
+		*buf = p;
+		*size = buf_size;
+		ret = efi_get_next_variable_name_int(&buf_size, *buf, guid);
+	}
+
+	return ret;
+}
-- 
2.17.1

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

[Masahisa Kojima]

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 retrieved all EFI variables, it should be treated
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@linaro.org>
---
Changes in v2:
- fix typos
- use '!guidcmp()' instead of 'guidcmp() == 0'
- remove superfluous malloc() branch

 cmd/eficonfig.c | 54 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 0b07dfc958..ce7175a566 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_next_variable_name(&buf_size, &var_name16, &guid);
-		if (ret == EFI_NOT_FOUND)
+		if (ret == EFI_NOT_FOUND) {
+			/*
+			 * EFI_NOT_FOUND indicates we retrieved all EFI variables.
+			 * This should be treated as success.
+			 */
+			ret = EFI_SUCCESS;
 			break;
+		}
 		if (ret != EFI_SUCCESS)
 			goto out;
 
@@ -2349,31 +2356,46 @@ 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)) {
+			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);
-						goto out;
-					}
+			/*
+			 * 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) {
+				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;
 }
-- 
2.17.1

Re: [PATCH v2 1/2] eficonfig: carve out efi_get_next_variable_name_int calls

[Heinrich Schuchardt]

On 12/19/22 02:33, Masahisa Kojima wrote:
> To retrieve the EFI variable name by efi_get_next_variable_name_int(),
> the sequence of alloc -> efi_get_next_variable_name_int ->
> realloc -> efi_get_next_variable_name_int is required.
> In current code, this sequence repeatedly appears in
> the several functions. It should be curved out a common function.
> 
> This commit also fixes the missing free() of var_name16
> in eficonfig_delete_invalid_boot_option().
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

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

[Heinrich Schuchardt]

On 12/19/22 02:33, 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 retrieved all EFI variables, it should be treated
> 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@linaro.org>

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Re: [PATCH v2 1/2] eficonfig: carve out efi_get_next_variable_name_int calls

[Ilias Apalodimas]

On Mon, Dec 19, 2022 at 11:33:12AM +0900, Masahisa Kojima wrote:
> To retrieve the EFI variable name by efi_get_next_variable_name_int(),
> the sequence of alloc -> efi_get_next_variable_name_int ->
> realloc -> efi_get_next_variable_name_int is required.
> In current code, this sequence repeatedly appears in
> the several functions. It should be curved out a common function.
>
> This commit also fixes the missing free() of var_name16
> in eficonfig_delete_invalid_boot_option().
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v2:
> - fix typo in the commit message
> - rename efi_get_variable_name to efi_next_variable_name
>
>  cmd/eficonfig.c             | 62 +++++++++----------------------------
>  include/efi_loader.h        |  2 ++
>  lib/efi_loader/efi_helper.c | 34 ++++++++++++++++++++
>  3 files changed, 51 insertions(+), 47 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 394ae67cce..0b07dfc958 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -1683,7 +1683,7 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
>  	u32 i;
>  	u16 *bootorder;
>  	efi_status_t ret;
> -	u16 *var_name16 = NULL, *p;
> +	u16 *var_name16 = NULL;
>  	efi_uintn_t num, size, buf_size;
>  	struct efimenu *efi_menu;
>  	struct list_head *pos, *n;
> @@ -1718,24 +1718,12 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
>  		int index;
>  		efi_guid_t guid;
>
> -		size = buf_size;
> -		ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> +		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
>  		if (ret == EFI_NOT_FOUND)
>  			break;
> -		if (ret == EFI_BUFFER_TOO_SMALL) {
> -			buf_size = size;
> -			p = realloc(var_name16, buf_size);
> -			if (!p) {
> -				free(var_name16);
> -				return EFI_OUT_OF_RESOURCES;
> -			}
> -			var_name16 = p;
> -			ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> -		}
> -		if (ret != EFI_SUCCESS) {
> -			free(var_name16);
> -			return ret;
> -		}
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +
>  		if (efi_varname_is_load_option(var_name16, &index)) {
>  			/* If the index is included in the BootOrder, skip it */
>  			if (search_bootorder(bootorder, num, index, NULL))
> @@ -2026,7 +2014,7 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
>  	u32 i;
>  	char *title;
>  	efi_status_t ret;
> -	u16 *var_name16 = NULL, *p;
> +	u16 *var_name16 = NULL;
>  	efi_uintn_t size, buf_size;
>
>  	/* list the load option in the order of BootOrder variable */
> @@ -2054,19 +2042,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
>  			break;
>
>  		size = buf_size;
> -		ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> +		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
>  		if (ret == EFI_NOT_FOUND)
>  			break;
> -		if (ret == EFI_BUFFER_TOO_SMALL) {
> -			buf_size = size;
> -			p = realloc(var_name16, buf_size);
> -			if (!p) {
> -				ret = EFI_OUT_OF_RESOURCES;
> -				goto out;
> -			}
> -			var_name16 = p;
> -			ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> -		}
>  		if (ret != EFI_SUCCESS)
>  			goto out;
>
> @@ -2336,10 +2314,10 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
>  	efi_uintn_t size;
>  	void *load_option;
>  	struct efi_load_option lo;
> -	u16 *var_name16 = NULL, *p;
> +	u16 *var_name16 = NULL;
>  	u16 varname[] = u"Boot####";
>  	efi_status_t ret = EFI_SUCCESS;
> -	efi_uintn_t varname_size, buf_size;
> +	efi_uintn_t buf_size;
>
>  	buf_size = 128;
>  	var_name16 = malloc(buf_size);
> @@ -2352,24 +2330,12 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
>  		efi_guid_t guid;
>  		efi_uintn_t tmp;
>
> -		varname_size = buf_size;
> -		ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid);
> +		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
>  		if (ret == EFI_NOT_FOUND)
>  			break;
> -		if (ret == EFI_BUFFER_TOO_SMALL) {
> -			buf_size = varname_size;
> -			p = realloc(var_name16, buf_size);
> -			if (!p) {
> -				free(var_name16);
> -				return EFI_OUT_OF_RESOURCES;
> -			}
> -			var_name16 = p;
> -			ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid);
> -		}
> -		if (ret != EFI_SUCCESS) {
> -			free(var_name16);
> -			return ret;
> -		}
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +
>  		if (!efi_varname_is_load_option(var_name16, &index))
>  			continue;
>
> @@ -2407,6 +2373,8 @@ next:
>  	}
>
>  out:
> +	free(var_name16);
> +
>  	return ret;
>  }
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0899e293e5..699176872d 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -708,6 +708,8 @@ int algo_to_len(const char *algo);
>  int efi_link_dev(efi_handle_t handle, struct udevice *dev);
>  int efi_unlink_dev(efi_handle_t handle);
>  bool efi_varname_is_load_option(u16 *var_name16, int *index);
> +efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> +				    efi_guid_t *guid);
>
>  /**
>   * efi_size_in_pages() - convert size in bytes to size in pages
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 788cb9faec..1f4ab2b419 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -223,3 +223,37 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index)
>
>  	return false;
>  }
> +
> +/**
> + * efi_next_variable_name() - get next variable name
> + *
> + * This function is a wrapper of efi_get_next_variable_name_int().
> + * If efi_get_next_variable_name_int() returns EFI_BUFFER_TOO_SMALL,
> + * @size and @buf are updated by new buffer size and realloced buffer.
> + *
> + * @size:	pointer to the buffer size
> + * @buf:	pointer to the buffer
> + * @guid:	pointer to the guid
> + * Return:	status code
> + */
> +efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, efi_guid_t *guid)
> +{
> +	u16 *p;
> +	efi_status_t ret;
> +	efi_uintn_t buf_size = *size;
> +
> +	ret = efi_get_next_variable_name_int(&buf_size, *buf, guid);
> +	if (ret == EFI_NOT_FOUND)
> +		return ret;
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		p = realloc(*buf, buf_size);
> +		if (!p)
> +			return EFI_OUT_OF_RESOURCES;
> +
> +		*buf = p;
> +		*size = buf_size;
> +		ret = efi_get_next_variable_name_int(&buf_size, *buf, guid);
> +	}
> +
> +	return ret;
> +}
> --
> 2.17.1
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

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

[Ilias Apalodimas]

On Mon, Dec 19, 2022 at 11:33:13AM +0900, 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 retrieved all EFI variables, it should be treated
> 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@linaro.org>
> ---
> Changes in v2:
> - fix typos
> - use '!guidcmp()' instead of 'guidcmp() == 0'
> - remove superfluous malloc() branch
>
>  cmd/eficonfig.c | 54 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 0b07dfc958..ce7175a566 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_next_variable_name(&buf_size, &var_name16, &guid);
> -		if (ret == EFI_NOT_FOUND)
> +		if (ret == EFI_NOT_FOUND) {
> +			/*
> +			 * EFI_NOT_FOUND indicates we retrieved all EFI variables.
> +			 * This should be treated as success.
> +			 */
> +			ret = EFI_SUCCESS;
>  			break;
> +		}
>  		if (ret != EFI_SUCCESS)
>  			goto out;
>
> @@ -2349,31 +2356,46 @@ 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)) {
> +			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);
> -						goto out;
> -					}
> +			/*
> +			 * 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) {
> +				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;
>  }
> --
> 2.17.1
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>