* [PATCH 0/2] fix eficonfig GetNextVariableName calls handling
@ 2022-12-08 13:40 Masahisa Kojima
2022-12-08 13:40 ` [PATCH 1/2] eficonfig: curve out efi_get_next_variable_name_int calls Masahisa Kojima
2022-12-08 13:40 ` [PATCH 2/2] eficonfig: avoid SetVariable between GetNextVariableName calls Masahisa Kojima
0 siblings, 2 replies; 7+ messages in thread
From: Masahisa Kojima @ 2022-12-08 13:40 UTC (permalink / raw
To: u-boot; +Cc: Heinrich Schuchardt, Ilias Apalodimas, Masahisa Kojima
This series includes refactoring and bugfix of GetNextVariableName
calls in eficonfig.
After "eficonfig: curve 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: curve out efi_get_next_variable_name_int calls
eficonfig: avoid SetVariable between GetNextVariableName calls
cmd/eficonfig.c | 119 ++++++++++++++++++------------------
include/efi_loader.h | 2 +
lib/efi_loader/efi_helper.c | 34 +++++++++++
3 files changed, 95 insertions(+), 60 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] eficonfig: curve out efi_get_next_variable_name_int calls
2022-12-08 13:40 [PATCH 0/2] fix eficonfig GetNextVariableName calls handling Masahisa Kojima
@ 2022-12-08 13:40 ` Masahisa Kojima
2022-12-16 1:51 ` Heinrich Schuchardt
2022-12-08 13:40 ` [PATCH 2/2] eficonfig: avoid SetVariable between GetNextVariableName calls Masahisa Kojima
1 sibling, 1 reply; 7+ messages in thread
From: Masahisa Kojima @ 2022-12-08 13:40 UTC (permalink / raw
To: u-boot; +Cc: Heinrich Schuchardt, Ilias Apalodimas, 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>
---
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;
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);
@@ -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)
+{
+ 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] eficonfig: avoid SetVariable between GetNextVariableName calls
2022-12-08 13:40 [PATCH 0/2] fix eficonfig GetNextVariableName calls handling Masahisa Kojima
2022-12-08 13:40 ` [PATCH 1/2] eficonfig: curve out efi_get_next_variable_name_int calls Masahisa Kojima
@ 2022-12-08 13:40 ` Masahisa Kojima
2022-12-16 1:58 ` Heinrich Schuchardt
1 sibling, 1 reply; 7+ messages in thread
From: Masahisa Kojima @ 2022-12-08 13:40 UTC (permalink / raw
To: u-boot; +Cc: Heinrich Schuchardt, Ilias Apalodimas, 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 retrieve 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>
---
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,
+ * should be treated as success.
+ */
+ 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) {
+ 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));
+ 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;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] eficonfig: curve out efi_get_next_variable_name_int calls
2022-12-08 13:40 ` [PATCH 1/2] eficonfig: curve out efi_get_next_variable_name_int calls Masahisa Kojima
@ 2022-12-16 1:51 ` Heinrich Schuchardt
2022-12-16 6:22 ` Masahisa Kojima
0 siblings, 1 reply; 7+ messages in thread
From: Heinrich Schuchardt @ 2022-12-16 1:51 UTC (permalink / raw
To: Masahisa Kojima; +Cc: Ilias Apalodimas, u-boot
%s/curve/carve/
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.
> @@ -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".
> +{
> + 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.
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;
> +}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] eficonfig: avoid SetVariable between GetNextVariableName calls
2022-12-08 13:40 ` [PATCH 2/2] eficonfig: avoid SetVariable between GetNextVariableName calls Masahisa Kojima
@ 2022-12-16 1:58 ` Heinrich Schuchardt
2022-12-16 6:48 ` Masahisa Kojima
0 siblings, 1 reply; 7+ messages in thread
From: Heinrich Schuchardt @ 2022-12-16 1:58 UTC (permalink / raw
To: Masahisa Kojima; +Cc: Ilias Apalodimas, u-boot
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;
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] eficonfig: curve out efi_get_next_variable_name_int calls
2022-12-16 1:51 ` Heinrich Schuchardt
@ 2022-12-16 6:22 ` Masahisa Kojima
0 siblings, 0 replies; 7+ messages in thread
From: Masahisa Kojima @ 2022-12-16 6:22 UTC (permalink / raw
To: Heinrich Schuchardt; +Cc: Ilias Apalodimas, u-boot
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;
> > +}
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] eficonfig: avoid SetVariable between GetNextVariableName calls
2022-12-16 1:58 ` Heinrich Schuchardt
@ 2022-12-16 6:48 ` Masahisa Kojima
0 siblings, 0 replies; 7+ messages in thread
From: Masahisa Kojima @ 2022-12-16 6:48 UTC (permalink / raw
To: Heinrich Schuchardt; +Cc: Ilias Apalodimas, u-boot
On Fri, 16 Dec 2022 at 10:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> 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/
OK.
>
> > 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/
OK.
>
> > + * should be treated as success.
>
> This should be ...
OK.
>
> > + */
> > + 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'.
OK.
>
> > + 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.
OK, thank you.
Regards,
Masahisa Kojima
>
> 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;
> > }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-12-16 6:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-08 13:40 [PATCH 0/2] fix eficonfig GetNextVariableName calls handling Masahisa Kojima
2022-12-08 13:40 ` [PATCH 1/2] eficonfig: curve out efi_get_next_variable_name_int calls Masahisa Kojima
2022-12-16 1:51 ` Heinrich Schuchardt
2022-12-16 6:22 ` Masahisa Kojima
2022-12-08 13:40 ` [PATCH 2/2] eficonfig: avoid SetVariable between GetNextVariableName calls Masahisa Kojima
2022-12-16 1:58 ` Heinrich Schuchardt
2022-12-16 6:48 ` Masahisa Kojima
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.