[PATCH v2 0/2] fix eficonfig GetNextVariableName calls handling
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(-)
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; +}
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
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_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_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);}@@ -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_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
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; }
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 Kojimamasahisa.kojima@linaro.org
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
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;}@@ -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
participants (3)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Masahisa Kojima