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

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