On Fri, 16 Dec 2022 at 10:51, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

%s/curve/carve/

Thank you for pointing out the typo.

On 12/8/22 05:40, 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

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..cd7a51cc7e 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;
  

buf_size = 0; see below

...

  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_get_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_get_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);
  

Why allocate anything here? We already have a realloc() in efi_get_variable_name() which will do the job. Just start with var_name16 = NULL, buf_size = 0.

To start the EFI variable search with GetNextVariableName(), the first call of efi_get_next_variable_name_int() requires a pointer to the Null-terminated string and non-zero buffer size. So var_name16 = NULL, buf_size = 0 setup will not work as expected, efi_get_next_variable_name_int() returns EFI_INVALID_PARAMETER in this case.

...

@@ -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_get_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..f80a16108a 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_get_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..ca9854ec79 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_get_variable_name_() - get 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_get_variable_name(efi_uintn_t *size, u16 **buf, efi_guid_t *guid)

We should have next in the function name, e.g. "efi_next_variable_name".

Yes, I agree.

...

+{

• 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)
  

*buf should be freed here and set to NULL.

var_name16 should be allocated by a caller as described above, let me keep the current code.

Thanks, Masahisa Kojima

Best regards

Heinrich

...

•                 return EFI_OUT_OF_RESOURCES;
  

•         *buf = p;
  

•         *size = buf_size;
  

•         ret = efi_get_next_variable_name_int(&buf_size, *buf, guid);
  

• }
  

• return ret;
  

+}

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@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; }