[PATCH v3 0/5] miscellaneous fixes of eficonfig
This series includes bugfix, refactoring and documentation updates.
Masahisa Kojima (5): eficonfig: fix going one directory up issue eficonfig: use u16_strsize() to get u16 string buffer size efi_loader: utility function to check the variable name is "Boot####" eficonfig: use efi_get_next_variable_name_int() doc:eficonfig: add description for UEFI Secure Boot Configuration
cmd/eficonfig.c | 146 +++++++++++++++++++++++++++++------- cmd/efidebug.c | 23 +----- doc/usage/cmd/eficonfig.rst | 22 ++++++ include/efi_loader.h | 1 + lib/efi_loader/efi_helper.c | 33 ++++++++ 5 files changed, 176 insertions(+), 49 deletions(-)
The directory name in eficonfig menu entry contains the '' separator. strcmp() argument ".." is wrong and one directory up handling does not work correctly. strcmp() argument must include '' separator.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- No change since v1
cmd/eficonfig.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 97d35597a2..5529edc85e 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -488,7 +488,7 @@ static efi_status_t eficonfig_file_selected(void *data) if (!info) return EFI_INVALID_PARAMETER;
- if (!strcmp(info->file_name, "..")) { + if (!strcmp(info->file_name, "..\")) { struct eficonfig_filepath_info *iter; struct list_head *pos, *n; int is_last;
Use u16_strsize() to simplify the u16 string buffer size calculation.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- No update since v1.
cmd/eficonfig.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 5529edc85e..88d507d04c 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -452,8 +452,7 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_ struct efi_device_path *dp; struct efi_device_path_file_path *fp;
- fp_size = sizeof(struct efi_device_path) + - ((u16_strlen(current_path) + 1) * sizeof(u16)); + fp_size = sizeof(struct efi_device_path) + u16_strsize(current_path); buf = calloc(1, fp_size + sizeof(END)); if (!buf) return NULL;
Some commands need to enumerate the existing UEFI load option variable("Boot####"). This commit transfers some code from cmd/efidebug.c to lib/efi_loder/, then exposes efi_varname_is_load_option() function to check whether the UEFI variable name is "Boot####".
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- v2->v3: - add static qualifier to u16_tohex()
Newly created in v2
cmd/efidebug.c | 23 +---------------------- include/efi_loader.h | 1 + lib/efi_loader/efi_helper.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 22 deletions(-)
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index ef239bb34b..ceb3aa5cee 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -1010,17 +1010,6 @@ static void show_efi_boot_opt(u16 *varname16) } }
-static int u16_tohex(u16 c) -{ - if (c >= '0' && c <= '9') - return c - '0'; - if (c >= 'A' && c <= 'F') - return c - 'A' + 10; - - /* not hexadecimal */ - return -1; -} - /** * show_efi_boot_dump() - dump all UEFI load options * @@ -1041,7 +1030,6 @@ static int do_efi_boot_dump(struct cmd_tbl *cmdtp, int flag, u16 *var_name16, *p; efi_uintn_t buf_size, size; efi_guid_t guid; - int id, i, digit; efi_status_t ret;
if (argc > 1) @@ -1074,16 +1062,7 @@ static int do_efi_boot_dump(struct cmd_tbl *cmdtp, int flag, return CMD_RET_FAILURE; }
- if (memcmp(var_name16, u"Boot", 8)) - continue; - - for (id = 0, i = 0; i < 4; i++) { - digit = u16_tohex(var_name16[4 + i]); - if (digit < 0) - break; - id = (id << 4) + digit; - } - if (i == 4 && !var_name16[8]) + if (efi_varname_is_load_option(var_name16, NULL)) show_efi_boot_opt(var_name16); }
diff --git a/include/efi_loader.h b/include/efi_loader.h index 0c6c95ba46..0899e293e5 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -707,6 +707,7 @@ 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_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 c71e87d118..788cb9faec 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -190,3 +190,36 @@ int efi_unlink_dev(efi_handle_t handle)
return 0; } + +static int u16_tohex(u16 c) +{ + if (c >= '0' && c <= '9') + return c - '0'; + if (c >= 'A' && c <= 'F') + return c - 'A' + 10; + + /* not hexadecimal */ + return -1; +} + +bool efi_varname_is_load_option(u16 *var_name16, int *index) +{ + int id, i, digit; + + if (memcmp(var_name16, u"Boot", 8)) + return false; + + for (id = 0, i = 0; i < 4; i++) { + digit = u16_tohex(var_name16[4 + i]); + if (digit < 0) + break; + id = (id << 4) + digit; + } + if (i == 4 && !var_name16[8]) { + if (index) + *index = id; + return true; + } + + return false; +}
eficonfig command reads all possible UEFI load options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment. This commit uses efi_get_next_variable_name_int() to read all existing UEFI load options to significantlly reduce the count of efi_get_var() call.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- No update since v2
v1->v2: - totaly change the implemention, remove new Kconfig introduced in v1. - use efi_get_next_variable_name_int() to read the all existing UEFI variables, then enumerate the "Boot####" variables - this commit does not provide the common function to enumerate all "Boot####" variables. I think common function does not simplify the implementation, because caller of efi_get_next_variable_name_int() needs to remember original buffer size, new buffer size and buffer address and it is a bit complicated when we consider to create common function.
cmd/eficonfig.c | 141 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 117 insertions(+), 24 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 88d507d04c..394ae67cce 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1683,7 +1683,8 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) u32 i; u16 *bootorder; efi_status_t ret; - efi_uintn_t num, size; + u16 *var_name16 = NULL, *p; + efi_uintn_t num, size, buf_size; struct efimenu *efi_menu; struct list_head *pos, *n; struct eficonfig_entry *entry; @@ -1707,14 +1708,43 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) }
/* list the remaining load option not included in the BootOrder */ - for (i = 0; i <= 0xFFFF; i++) { - /* If the index is included in the BootOrder, skip it */ - if (search_bootorder(bootorder, num, i, NULL)) - continue; + buf_size = 128; + var_name16 = malloc(buf_size); + if (!var_name16) + return EFI_OUT_OF_RESOURCES;
- ret = eficonfig_add_boot_selection_entry(efi_menu, i, selected); - if (ret != EFI_SUCCESS) - goto out; + var_name16[0] = 0; + for (;;) { + int index; + efi_guid_t guid; + + size = buf_size; + ret = efi_get_next_variable_name_int(&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 (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)) + continue; + + ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected); + if (ret != EFI_SUCCESS) + goto out; + }
if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 1) break; @@ -1732,6 +1762,8 @@ out: } eficonfig_destroy(efi_menu);
+ free(var_name16); + return ret; }
@@ -1994,6 +2026,8 @@ 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; + efi_uintn_t size, buf_size;
/* list the load option in the order of BootOrder variable */ for (i = 0; i < num; i++) { @@ -2006,17 +2040,45 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi }
/* list the remaining load option not included in the BootOrder */ - for (i = 0; i < 0xFFFF; i++) { + buf_size = 128; + var_name16 = malloc(buf_size); + if (!var_name16) + return EFI_OUT_OF_RESOURCES; + + var_name16[0] = 0; + for (;;) { + int index; + efi_guid_t guid; + if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;
- /* If the index is included in the BootOrder, skip it */ - if (search_bootorder(bootorder, num, i, NULL)) - continue; - - ret = eficonfig_add_change_boot_order_entry(efi_menu, i, false); + size = buf_size; + ret = efi_get_next_variable_name_int(&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; + + 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)) + continue; + + ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false); + if (ret != EFI_SUCCESS) + goto out; + } }
/* add "Save" and "Quit" entries */ @@ -2035,9 +2097,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi goto out;
efi_menu->active = 0; - - return EFI_SUCCESS; out: + free(var_name16); + return ret; }
@@ -2270,17 +2332,48 @@ out: efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt, efi_status_t count) { - u32 i, j; + u32 i; efi_uintn_t size; void *load_option; struct efi_load_option lo; + u16 *var_name16 = NULL, *p; u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS; + efi_uintn_t varname_size, buf_size;
- for (i = 0; i <= 0xFFFF; i++) { + buf_size = 128; + var_name16 = malloc(buf_size); + if (!var_name16) + return EFI_OUT_OF_RESOURCES; + + var_name16[0] = 0; + for (;;) { + int index; + efi_guid_t guid; efi_uintn_t tmp;
- efi_create_indexed_name(varname, sizeof(varname), "Boot", i); + varname_size = buf_size; + ret = efi_get_next_variable_name_int(&varname_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 (!efi_varname_is_load_option(var_name16, &index)) + continue; + + efi_create_indexed_name(varname, sizeof(varname), "Boot", index); load_option = efi_get_var(varname, &efi_global_variable_guid, &size); if (!load_option) continue; @@ -2292,15 +2385,15 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
if (size >= sizeof(efi_guid_bootmenu_auto_generated)) { if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) { - for (j = 0; j < count; j++) { - if (opt[j].size == tmp && - memcmp(opt[j].lo, load_option, tmp) == 0) { - opt[j].exist = true; + 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 (j == count) { + if (i == count) { ret = delete_boot_option(i); if (ret != EFI_SUCCESS) { free(load_option);
On Fri, Dec 02, 2022 at 01:59:36PM +0900, Masahisa Kojima wrote:
eficonfig command reads all possible UEFI load options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment. This commit uses efi_get_next_variable_name_int() to read all existing UEFI load options to significantlly reduce the count of efi_get_var() call.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
No update since v2
v1->v2:
- totaly change the implemention, remove new Kconfig introduced in v1.
- use efi_get_next_variable_name_int() to read the all existing UEFI variables, then enumerate the "Boot####" variables
- this commit does not provide the common function to enumerate all "Boot####" variables. I think common function does not simplify the implementation, because caller of efi_get_next_variable_name_int() needs to remember original buffer size, new buffer size and buffer address and it is a bit complicated when we consider to create common function.
cmd/eficonfig.c | 141 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 117 insertions(+), 24 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 88d507d04c..394ae67cce 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1683,7 +1683,8 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) u32 i; u16 *bootorder; efi_status_t ret;
- efi_uintn_t num, size;
- u16 *var_name16 = NULL, *p;
- efi_uintn_t num, size, buf_size; struct efimenu *efi_menu; struct list_head *pos, *n; struct eficonfig_entry *entry;
@@ -1707,14 +1708,43 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) }
/* list the remaining load option not included in the BootOrder */
- for (i = 0; i <= 0xFFFF; i++) {
/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;
- buf_size = 128;
- var_name16 = malloc(buf_size);
- if (!var_name16)
return EFI_OUT_OF_RESOURCES;
ret = eficonfig_add_boot_selection_entry(efi_menu, i, selected);if (ret != EFI_SUCCESS)goto out;
- var_name16[0] = 0;
Is it worth using calloc instead of malloc and get rid of this?
for (;;) {
int index;efi_guid_t guid;size = buf_size;ret = efi_get_next_variable_name_int(&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 (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))continue;ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected);if (ret != EFI_SUCCESS)goto out;}if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 1) break;
@@ -1732,6 +1762,8 @@ out: } eficonfig_destroy(efi_menu);
- free(var_name16);
- return ret;
}
@@ -1994,6 +2026,8 @@ 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;
efi_uintn_t size, buf_size;
/* list the load option in the order of BootOrder variable */ for (i = 0; i < num; i++) {
@@ -2006,17 +2040,45 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi }
/* list the remaining load option not included in the BootOrder */
- for (i = 0; i < 0xFFFF; i++) {
- buf_size = 128;
- var_name16 = malloc(buf_size);
- if (!var_name16)
return EFI_OUT_OF_RESOURCES;- var_name16[0] = 0;
- for (;;) {
int index;efi_guid_t guid;- if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;
/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, i, false);
size = buf_size;ret = efi_get_next_variable_name_int(&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;
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))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false);if (ret != EFI_SUCCESS)goto out;}}
/* add "Save" and "Quit" entries */
@@ -2035,9 +2097,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi goto out;
efi_menu->active = 0;
- return EFI_SUCCESS;
out:
- free(var_name16);
- return ret;
}
@@ -2270,17 +2332,48 @@ out: efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt, efi_status_t count) {
- u32 i, j;
- u32 i; efi_uintn_t size; void *load_option; struct efi_load_option lo;
- u16 *var_name16 = NULL, *p; u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS;
- efi_uintn_t varname_size, buf_size;
- for (i = 0; i <= 0xFFFF; i++) {
- buf_size = 128;
- var_name16 = malloc(buf_size);
- if (!var_name16)
return EFI_OUT_OF_RESOURCES;- var_name16[0] = 0;
- for (;;) {
int index;efi_guid_t guid;
efi_create_indexed_name(varname, sizeof(varname), "Boot", i);
varname_size = buf_size;ret = efi_get_next_variable_name_int(&varname_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 (!efi_varname_is_load_option(var_name16, &index))continue;efi_create_indexed_name(varname, sizeof(varname), "Boot", index);@@ -2292,15 +2385,15 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
if (size >= sizeof(efi_guid_bootmenu_auto_generated)) { if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) {
for (j = 0; j < count; j++) {if (opt[j].size == tmp &&memcmp(opt[j].lo, load_option, tmp) == 0) {opt[j].exist = true;
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 (j == count) {
if (i == count) { ret = delete_boot_option(i); if (ret != EFI_SUCCESS) { free(load_option);-- 2.17.1
Overall this looks correct and a good idea. The sequence of alloc -> efi_get_next_variable_name_int -> realloc -> efi_get_next_variable_name_int seems common and repeated though. Can we factor that out on a common function?
Thanks /Ilias
On 12/2/22 08:35, Ilias Apalodimas wrote:
On Fri, Dec 02, 2022 at 01:59:36PM +0900, Masahisa Kojima wrote:
eficonfig command reads all possible UEFI load options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment. This commit uses efi_get_next_variable_name_int() to read all existing UEFI load options to significantlly reduce the count of efi_get_var() call.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
No update since v2
v1->v2:
totaly change the implemention, remove new Kconfig introduced in v1.
use efi_get_next_variable_name_int() to read the all existing UEFI variables, then enumerate the "Boot####" variables
this commit does not provide the common function to enumerate all "Boot####" variables. I think common function does not simplify the implementation, because caller of efi_get_next_variable_name_int() needs to remember original buffer size, new buffer size and buffer address and it is a bit complicated when we consider to create common function.
cmd/eficonfig.c | 141 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 117 insertions(+), 24 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 88d507d04c..394ae67cce 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1683,7 +1683,8 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) u32 i; u16 *bootorder; efi_status_t ret;
- efi_uintn_t num, size;
- u16 *var_name16 = NULL, *p;
- efi_uintn_t num, size, buf_size; struct efimenu *efi_menu; struct list_head *pos, *n; struct eficonfig_entry *entry;
@@ -1707,14 +1708,43 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) }
/* list the remaining load option not included in the BootOrder */
- for (i = 0; i <= 0xFFFF; i++) {
/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;
- buf_size = 128;
- var_name16 = malloc(buf_size);
- if (!var_name16)
return EFI_OUT_OF_RESOURCES;
ret = eficonfig_add_boot_selection_entry(efi_menu, i, selected);if (ret != EFI_SUCCESS)goto out;
- var_name16[0] = 0;
Is it worth using calloc instead of malloc and get rid of this?
It doesn't change the binary code size as var_name16 is already in a register.
Best regards
Heinrich
for (;;) {
int index;efi_guid_t guid;size = buf_size;ret = efi_get_next_variable_name_int(&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 (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))continue;ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected);if (ret != EFI_SUCCESS)goto out;}if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 1) break;
@@ -1732,6 +1762,8 @@ out: } eficonfig_destroy(efi_menu);
- free(var_name16);
- return ret; }
@@ -1994,6 +2026,8 @@ 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;
efi_uintn_t size, buf_size;
/* list the load option in the order of BootOrder variable */ for (i = 0; i < num; i++) {
@@ -2006,17 +2040,45 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi }
/* list the remaining load option not included in the BootOrder */
- for (i = 0; i < 0xFFFF; i++) {
- buf_size = 128;
- var_name16 = malloc(buf_size);
- if (!var_name16)
return EFI_OUT_OF_RESOURCES;- var_name16[0] = 0;
- for (;;) {
int index;efi_guid_t guid;- if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;
/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, i, false);
size = buf_size;ret = efi_get_next_variable_name_int(&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;
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))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false);if (ret != EFI_SUCCESS)goto out;}}
/* add "Save" and "Quit" entries */
@@ -2035,9 +2097,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi goto out;
efi_menu->active = 0;
- return EFI_SUCCESS; out:
- free(var_name16);
- return ret; }
@@ -2270,17 +2332,48 @@ out: efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt, efi_status_t count) {
- u32 i, j;
- u32 i; efi_uintn_t size; void *load_option; struct efi_load_option lo;
- u16 *var_name16 = NULL, *p; u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS;
- efi_uintn_t varname_size, buf_size;
- for (i = 0; i <= 0xFFFF; i++) {
- buf_size = 128;
- var_name16 = malloc(buf_size);
- if (!var_name16)
return EFI_OUT_OF_RESOURCES;- var_name16[0] = 0;
- for (;;) {
int index;efi_guid_t guid;
efi_create_indexed_name(varname, sizeof(varname), "Boot", i);
varname_size = buf_size;ret = efi_get_next_variable_name_int(&varname_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 (!efi_varname_is_load_option(var_name16, &index))continue;efi_create_indexed_name(varname, sizeof(varname), "Boot", index);@@ -2292,15 +2385,15 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
if (size >= sizeof(efi_guid_bootmenu_auto_generated)) { if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) {
for (j = 0; j < count; j++) {if (opt[j].size == tmp &&memcmp(opt[j].lo, load_option, tmp) == 0) {opt[j].exist = true;
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 (j == count) {
if (i == count) { ret = delete_boot_option(i); if (ret != EFI_SUCCESS) { free(load_option);-- 2.17.1
Overall this looks correct and a good idea. The sequence of alloc -> efi_get_next_variable_name_int -> realloc -> efi_get_next_variable_name_int seems common and repeated though. Can we factor that out on a common function?
Thanks /Ilias
On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Fri, Dec 02, 2022 at 01:59:36PM +0900, Masahisa Kojima wrote:
eficonfig command reads all possible UEFI load options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment. This commit uses efi_get_next_variable_name_int() to read all existing UEFI load options to significantlly reduce the count of efi_get_var() call.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
No update since v2
v1->v2:
- totaly change the implemention, remove new Kconfig introduced in v1.
- use efi_get_next_variable_name_int() to read the all existing UEFI variables, then enumerate the "Boot####" variables
- this commit does not provide the common function to enumerate all "Boot####" variables. I think common function does not simplify the implementation, because caller of efi_get_next_variable_name_int() needs to remember original buffer size, new buffer size and buffer address and it is a bit complicated when we consider to create common function.
cmd/eficonfig.c | 141 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 117 insertions(+), 24 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 88d507d04c..394ae67cce 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1683,7 +1683,8 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) u32 i; u16 *bootorder; efi_status_t ret;
efi_uintn_t num, size;
u16 *var_name16 = NULL, *p;efi_uintn_t num, size, buf_size; struct efimenu *efi_menu; struct list_head *pos, *n; struct eficonfig_entry *entry;@@ -1707,14 +1708,43 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) }
/* list the remaining load option not included in the BootOrder */
for (i = 0; i <= 0xFFFF; i++) {/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;
buf_size = 128;var_name16 = malloc(buf_size);if (!var_name16)return EFI_OUT_OF_RESOURCES;
ret = eficonfig_add_boot_selection_entry(efi_menu, i, selected);if (ret != EFI_SUCCESS)goto out;
var_name16[0] = 0;Is it worth using calloc instead of malloc and get rid of this?
for (;;) {int index;efi_guid_t guid;size = buf_size;ret = efi_get_next_variable_name_int(&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 (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))continue;ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected);if (ret != EFI_SUCCESS)goto out;} if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 1) break;@@ -1732,6 +1762,8 @@ out: } eficonfig_destroy(efi_menu);
free(var_name16);return ret;}
@@ -1994,6 +2026,8 @@ 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;efi_uintn_t size, buf_size; /* list the load option in the order of BootOrder variable */ for (i = 0; i < num; i++) {@@ -2006,17 +2040,45 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi }
/* list the remaining load option not included in the BootOrder */
for (i = 0; i < 0xFFFF; i++) {
buf_size = 128;var_name16 = malloc(buf_size);if (!var_name16)return EFI_OUT_OF_RESOURCES;var_name16[0] = 0;for (;;) {int index;efi_guid_t guid;if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;
/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, i, false);
size = buf_size;ret = efi_get_next_variable_name_int(&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;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))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false);if (ret != EFI_SUCCESS)goto out;} } /* add "Save" and "Quit" entries */@@ -2035,9 +2097,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi goto out;
efi_menu->active = 0;
return EFI_SUCCESS;out:
free(var_name16);return ret;}
@@ -2270,17 +2332,48 @@ out: efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt, efi_status_t count) {
u32 i, j;
u32 i; efi_uintn_t size; void *load_option; struct efi_load_option lo;u16 *var_name16 = NULL, *p; u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS;efi_uintn_t varname_size, buf_size;
for (i = 0; i <= 0xFFFF; i++) {
buf_size = 128;var_name16 = malloc(buf_size);if (!var_name16)return EFI_OUT_OF_RESOURCES;var_name16[0] = 0;for (;;) {int index;efi_guid_t guid; efi_uintn_t tmp;
efi_create_indexed_name(varname, sizeof(varname), "Boot", i);
varname_size = buf_size;ret = efi_get_next_variable_name_int(&varname_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 (!efi_varname_is_load_option(var_name16, &index))continue;efi_create_indexed_name(varname, sizeof(varname), "Boot", index); load_option = efi_get_var(varname, &efi_global_variable_guid, &size); if (!load_option) continue;@@ -2292,15 +2385,15 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
if (size >= sizeof(efi_guid_bootmenu_auto_generated)) { if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) {
for (j = 0; j < count; j++) {if (opt[j].size == tmp &&memcmp(opt[j].lo, load_option, tmp) == 0) {opt[j].exist = true;
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 (j == count) {
if (i == count) { ret = delete_boot_option(i); if (ret != EFI_SUCCESS) { free(load_option);-- 2.17.1
Overall this looks correct and a good idea. The sequence of alloc -> efi_get_next_variable_name_int -> realloc -> efi_get_next_variable_name_int seems common and repeated though. Can we factor that out on a common function?
I tried to factor out these sequences into a common function, but I could not find proper common function interface. Even if we factor out some of these sequences, the caller still should take care the cases of ret == EFI_NOT_FOUND and ret != EFI_SUCCESS. We can factor out the EFI_BUFFER_TOO_SMALL and realloc sequence, but I think it will not make the caller simpler.
Thanks, Masahisa Kojima
Thanks /Ilias
On Sat, Dec 03, 2022 at 09:56:20AM +0900, Masahisa Kojima wrote:
On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Fri, Dec 02, 2022 at 01:59:36PM +0900, Masahisa Kojima wrote:
eficonfig command reads all possible UEFI load options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment. This commit uses efi_get_next_variable_name_int() to read all existing UEFI load options to significantlly reduce the count of efi_get_var() call.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
No update since v2
v1->v2:
- totaly change the implemention, remove new Kconfig introduced in v1.
- use efi_get_next_variable_name_int() to read the all existing UEFI variables, then enumerate the "Boot####" variables
- this commit does not provide the common function to enumerate all "Boot####" variables. I think common function does not simplify the implementation, because caller of efi_get_next_variable_name_int() needs to remember original buffer size, new buffer size and buffer address and it is a bit complicated when we consider to create common function.
cmd/eficonfig.c | 141 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 117 insertions(+), 24 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 88d507d04c..394ae67cce 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1683,7 +1683,8 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) u32 i; u16 *bootorder; efi_status_t ret;
efi_uintn_t num, size;
u16 *var_name16 = NULL, *p;efi_uintn_t num, size, buf_size; struct efimenu *efi_menu; struct list_head *pos, *n; struct eficonfig_entry *entry;@@ -1707,14 +1708,43 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) }
/* list the remaining load option not included in the BootOrder */
for (i = 0; i <= 0xFFFF; i++) {/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;
buf_size = 128;var_name16 = malloc(buf_size);if (!var_name16)return EFI_OUT_OF_RESOURCES;
ret = eficonfig_add_boot_selection_entry(efi_menu, i, selected);if (ret != EFI_SUCCESS)goto out;
var_name16[0] = 0;Is it worth using calloc instead of malloc and get rid of this?
for (;;) {int index;efi_guid_t guid;size = buf_size;ret = efi_get_next_variable_name_int(&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 (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))continue;ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected);if (ret != EFI_SUCCESS)goto out;} if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 1) break;@@ -1732,6 +1762,8 @@ out: } eficonfig_destroy(efi_menu);
free(var_name16);return ret;}
@@ -1994,6 +2026,8 @@ 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;efi_uintn_t size, buf_size; /* list the load option in the order of BootOrder variable */ for (i = 0; i < num; i++) {@@ -2006,17 +2040,45 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi }
/* list the remaining load option not included in the BootOrder */
for (i = 0; i < 0xFFFF; i++) {
buf_size = 128;var_name16 = malloc(buf_size);if (!var_name16)return EFI_OUT_OF_RESOURCES;var_name16[0] = 0;for (;;) {int index;efi_guid_t guid;if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;
/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, i, false);
size = buf_size;ret = efi_get_next_variable_name_int(&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;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))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false);if (ret != EFI_SUCCESS)goto out;} } /* add "Save" and "Quit" entries */@@ -2035,9 +2097,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi goto out;
efi_menu->active = 0;
return EFI_SUCCESS;out:
free(var_name16);return ret;}
@@ -2270,17 +2332,48 @@ out: efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt, efi_status_t count) {
u32 i, j;
u32 i; efi_uintn_t size; void *load_option; struct efi_load_option lo;u16 *var_name16 = NULL, *p; u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS;efi_uintn_t varname_size, buf_size;
for (i = 0; i <= 0xFFFF; i++) {
buf_size = 128;var_name16 = malloc(buf_size);if (!var_name16)return EFI_OUT_OF_RESOURCES;var_name16[0] = 0;for (;;) {int index;efi_guid_t guid; efi_uintn_t tmp;
efi_create_indexed_name(varname, sizeof(varname), "Boot", i);
varname_size = buf_size;ret = efi_get_next_variable_name_int(&varname_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 (!efi_varname_is_load_option(var_name16, &index))continue;efi_create_indexed_name(varname, sizeof(varname), "Boot", index); load_option = efi_get_var(varname, &efi_global_variable_guid, &size); if (!load_option) continue;@@ -2292,15 +2385,15 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
if (size >= sizeof(efi_guid_bootmenu_auto_generated)) { if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) {
for (j = 0; j < count; j++) {if (opt[j].size == tmp &&memcmp(opt[j].lo, load_option, tmp) == 0) {opt[j].exist = true;
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 (j == count) {
if (i == count) { ret = delete_boot_option(i); if (ret != EFI_SUCCESS) { free(load_option);-- 2.17.1
Overall this looks correct and a good idea. The sequence of alloc -> efi_get_next_variable_name_int -> realloc -> efi_get_next_variable_name_int seems common and repeated though. Can we factor that out on a common function?
I tried to factor out these sequences into a common function, but I could not find proper common function interface. Even if we factor out some of these sequences, the caller still should take care the cases of ret == EFI_NOT_FOUND and ret != EFI_SUCCESS. We can factor out the EFI_BUFFER_TOO_SMALL and realloc sequence, but I think it will not make the caller simpler.
I think mean the same thing here, but let me make sure. This snippet seems like it could be it's own function, no?
ret = efi_get_next_variable_name_int(&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); }
Regards /Ilias
Thanks, Masahisa Kojima
Thanks /Ilias
On Tue, 6 Dec 2022 at 23:12, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Sat, Dec 03, 2022 at 09:56:20AM +0900, Masahisa Kojima wrote:
On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Fri, Dec 02, 2022 at 01:59:36PM +0900, Masahisa Kojima wrote:
eficonfig command reads all possible UEFI load options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment. This commit uses efi_get_next_variable_name_int() to read all existing UEFI load options to significantlly reduce the count of efi_get_var() call.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
No update since v2
v1->v2:
- totaly change the implemention, remove new Kconfig introduced in v1.
- use efi_get_next_variable_name_int() to read the all existing UEFI variables, then enumerate the "Boot####" variables
- this commit does not provide the common function to enumerate all "Boot####" variables. I think common function does not simplify the implementation, because caller of efi_get_next_variable_name_int() needs to remember original buffer size, new buffer size and buffer address and it is a bit complicated when we consider to create common function.
cmd/eficonfig.c | 141 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 117 insertions(+), 24 deletions(-)
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 88d507d04c..394ae67cce 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1683,7 +1683,8 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) u32 i; u16 *bootorder; efi_status_t ret;
efi_uintn_t num, size;
u16 *var_name16 = NULL, *p;efi_uintn_t num, size, buf_size; struct efimenu *efi_menu; struct list_head *pos, *n; struct eficonfig_entry *entry;@@ -1707,14 +1708,43 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) }
/* list the remaining load option not included in the BootOrder */
for (i = 0; i <= 0xFFFF; i++) {/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;
buf_size = 128;var_name16 = malloc(buf_size);if (!var_name16)return EFI_OUT_OF_RESOURCES;
ret = eficonfig_add_boot_selection_entry(efi_menu, i, selected);if (ret != EFI_SUCCESS)goto out;
var_name16[0] = 0;Is it worth using calloc instead of malloc and get rid of this?
for (;;) {int index;efi_guid_t guid;size = buf_size;ret = efi_get_next_variable_name_int(&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 (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))continue;ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected);if (ret != EFI_SUCCESS)goto out;} if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 1) break;@@ -1732,6 +1762,8 @@ out: } eficonfig_destroy(efi_menu);
free(var_name16);return ret;}
@@ -1994,6 +2026,8 @@ 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;efi_uintn_t size, buf_size; /* list the load option in the order of BootOrder variable */ for (i = 0; i < num; i++) {@@ -2006,17 +2040,45 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi }
/* list the remaining load option not included in the BootOrder */
for (i = 0; i < 0xFFFF; i++) {
buf_size = 128;var_name16 = malloc(buf_size);if (!var_name16)return EFI_OUT_OF_RESOURCES;var_name16[0] = 0;for (;;) {int index;efi_guid_t guid;if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;
/* If the index is included in the BootOrder, skip it */if (search_bootorder(bootorder, num, i, NULL))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, i, false);
size = buf_size;ret = efi_get_next_variable_name_int(&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;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))continue;ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false);if (ret != EFI_SUCCESS)goto out;} } /* add "Save" and "Quit" entries */@@ -2035,9 +2097,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi goto out;
efi_menu->active = 0;
return EFI_SUCCESS;out:
free(var_name16);return ret;}
@@ -2270,17 +2332,48 @@ out: efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt, efi_status_t count) {
u32 i, j;
u32 i; efi_uintn_t size; void *load_option; struct efi_load_option lo;u16 *var_name16 = NULL, *p; u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS;efi_uintn_t varname_size, buf_size;
for (i = 0; i <= 0xFFFF; i++) {
buf_size = 128;var_name16 = malloc(buf_size);if (!var_name16)return EFI_OUT_OF_RESOURCES;var_name16[0] = 0;for (;;) {int index;efi_guid_t guid; efi_uintn_t tmp;
efi_create_indexed_name(varname, sizeof(varname), "Boot", i);
varname_size = buf_size;ret = efi_get_next_variable_name_int(&varname_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 (!efi_varname_is_load_option(var_name16, &index))continue;efi_create_indexed_name(varname, sizeof(varname), "Boot", index); load_option = efi_get_var(varname, &efi_global_variable_guid, &size); if (!load_option) continue;@@ -2292,15 +2385,15 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
if (size >= sizeof(efi_guid_bootmenu_auto_generated)) { if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) {
for (j = 0; j < count; j++) {if (opt[j].size == tmp &&memcmp(opt[j].lo, load_option, tmp) == 0) {opt[j].exist = true;
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 (j == count) {
if (i == count) { ret = delete_boot_option(i); if (ret != EFI_SUCCESS) { free(load_option);-- 2.17.1
Overall this looks correct and a good idea. The sequence of alloc -> efi_get_next_variable_name_int -> realloc -> efi_get_next_variable_name_int seems common and repeated though. Can we factor that out on a common function?
I tried to factor out these sequences into a common function, but I could not find proper common function interface. Even if we factor out some of these sequences, the caller still should take care the cases of ret == EFI_NOT_FOUND and ret != EFI_SUCCESS. We can factor out the EFI_BUFFER_TOO_SMALL and realloc sequence, but I think it will not make the caller simpler.
I think mean the same thing here, but let me make sure. This snippet seems like it could be it's own function, no?
ret = efi_get_next_variable_name_int(&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); }
OK, I will create a common function. This patch was already merged, I will send a new patch.
Thanks, Masahisa Kojima
Regards /Ilias
Thanks, Masahisa Kojima
Thanks /Ilias
This commits add the description for the UEFI Secure Boot Configuration through the eficonfig menu.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- No update since v2
Newly created in v2
doc/usage/cmd/eficonfig.rst | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/doc/usage/cmd/eficonfig.rst b/doc/usage/cmd/eficonfig.rst index 340ebc80db..67c859964f 100644 --- a/doc/usage/cmd/eficonfig.rst +++ b/doc/usage/cmd/eficonfig.rst @@ -31,6 +31,9 @@ Change Boot Order Delete Boot Option Delete the UEFI Boot Option
+Secure Boot Configuration + Edit UEFI Secure Boot Configuration + Configuration -------------
@@ -44,6 +47,16 @@ U-Boot console. In this case, bootmenu can be used to invoke "eficonfig":: CONFIG_USE_PREBOOT=y CONFIG_PREBOOT="setenv bootmenu_0 UEFI Maintenance Menu=eficonfig"
+UEFI specification requires that UEFI Secure Boot Configuration (especially +for PK and KEK) is stored in non-volatile storage which is tamper resident. +CONFIG_EFI_MM_COMM_TEE is mandatory to provide the secure storage in U-Boot. +UEFI Secure Boot Configuration menu entry is enabled when the following +options are enabled:: + + CONFIG_EFI_SECURE_BOOT=y + CONFIG_EFI_MM_COMM_TEE=y + + How to boot the system with newly added UEFI Boot Option ''''''''''''''''''''''''''''''''''''''''''''''''''''''''
@@ -66,6 +79,15 @@ add "bootefi bootmgr" entry as a default or first bootmenu entry::
CONFIG_PREBOOT="setenv bootmenu_0 UEFI Boot Manager=bootefi bootmgr; setenv bootmenu_1 UEFI Maintenance Menu=eficonfig"
+UEFI Secure Boot Configuration +'''''''''''''''''''''''''''''' + +User can enroll PK, KEK, db and dbx by selecting file. +"eficonfig" command only accepts the signed EFI Signature List(s) +with an authenticated header, typically ".auth" file. +To clear the PK, KEK, db and dbx, user needs to enroll the null key +signed by PK or KEK. + See also -------- * :doc:`bootmenu<bootmenu>` provides a simple mechanism for creating menus with different boot items
On Fri, Dec 02, 2022 at 01:59:37PM +0900, Masahisa Kojima wrote:
This commits add the description for the UEFI Secure Boot Configuration through the eficonfig menu.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
No update since v2
Newly created in v2
doc/usage/cmd/eficonfig.rst | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/doc/usage/cmd/eficonfig.rst b/doc/usage/cmd/eficonfig.rst index 340ebc80db..67c859964f 100644 --- a/doc/usage/cmd/eficonfig.rst +++ b/doc/usage/cmd/eficonfig.rst @@ -31,6 +31,9 @@ Change Boot Order Delete Boot Option Delete the UEFI Boot Option
+Secure Boot Configuration
- Edit UEFI Secure Boot Configuration
Configuration
@@ -44,6 +47,16 @@ U-Boot console. In this case, bootmenu can be used to invoke "eficonfig":: CONFIG_USE_PREBOOT=y CONFIG_PREBOOT="setenv bootmenu_0 UEFI Maintenance Menu=eficonfig"
+UEFI specification requires that UEFI Secure Boot Configuration (especially +for PK and KEK) is stored in non-volatile storage which is tamper resident.
s/resident/resistant
+CONFIG_EFI_MM_COMM_TEE is mandatory to provide the secure storage in U-Boot.
Can we be a bit more clear here. Something along the lines of "The only way U-Boot can currently store EFI variables on a tamper resistant medium is via OP-TEE. The Kconfig option that enables that is CONFIG_EFI_MM_COMM_TEE and ends up storing EFI variables on an RPMB partition of an eMMC"
+UEFI Secure Boot Configuration menu entry is enabled when the following +options are enabled::
- CONFIG_EFI_SECURE_BOOT=y
- CONFIG_EFI_MM_COMM_TEE=y
How to boot the system with newly added UEFI Boot Option ''''''''''''''''''''''''''''''''''''''''''''''''''''''''
@@ -66,6 +79,15 @@ add "bootefi bootmgr" entry as a default or first bootmenu entry::
CONFIG_PREBOOT="setenv bootmenu_0 UEFI Boot Manager=bootefi bootmgr; setenv bootmenu_1 UEFI Maintenance Menu=eficonfig"+UEFI Secure Boot Configuration +''''''''''''''''''''''''''''''
+User can enroll PK, KEK, db and dbx by selecting file.
selecting a file
+"eficonfig" command only accepts the signed EFI Signature List(s) +with an authenticated header, typically ".auth" file. +To clear the PK, KEK, db and dbx, user needs to enroll the null key +signed by PK or KEK.
See also
- :doc:`bootmenu<bootmenu>` provides a simple mechanism for creating menus with different boot items
-- 2.17.1
Thanks /Ilias
Hi Ilias,
On Fri, 2 Dec 2022 at 16:17, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Fri, Dec 02, 2022 at 01:59:37PM +0900, Masahisa Kojima wrote:
This commits add the description for the UEFI Secure Boot Configuration through the eficonfig menu.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
No update since v2
Newly created in v2
doc/usage/cmd/eficonfig.rst | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/doc/usage/cmd/eficonfig.rst b/doc/usage/cmd/eficonfig.rst index 340ebc80db..67c859964f 100644 --- a/doc/usage/cmd/eficonfig.rst +++ b/doc/usage/cmd/eficonfig.rst @@ -31,6 +31,9 @@ Change Boot Order Delete Boot Option Delete the UEFI Boot Option
+Secure Boot Configuration
- Edit UEFI Secure Boot Configuration
Configuration
@@ -44,6 +47,16 @@ U-Boot console. In this case, bootmenu can be used to invoke "eficonfig":: CONFIG_USE_PREBOOT=y CONFIG_PREBOOT="setenv bootmenu_0 UEFI Maintenance Menu=eficonfig"
+UEFI specification requires that UEFI Secure Boot Configuration (especially +for PK and KEK) is stored in non-volatile storage which is tamper resident.
s/resident/resistant
Thank you for pointing out the typo.
+CONFIG_EFI_MM_COMM_TEE is mandatory to provide the secure storage in U-Boot.
Can we be a bit more clear here. Something along the lines of "The only way U-Boot can currently store EFI variables on a tamper resistant medium is via OP-TEE. The Kconfig option that enables that is CONFIG_EFI_MM_COMM_TEE and ends up storing EFI variables on an RPMB partition of an eMMC"
OK, it is clearer.
+UEFI Secure Boot Configuration menu entry is enabled when the following +options are enabled::
- CONFIG_EFI_SECURE_BOOT=y
- CONFIG_EFI_MM_COMM_TEE=y
How to boot the system with newly added UEFI Boot Option ''''''''''''''''''''''''''''''''''''''''''''''''''''''''
@@ -66,6 +79,15 @@ add "bootefi bootmgr" entry as a default or first bootmenu entry::
CONFIG_PREBOOT="setenv bootmenu_0 UEFI Boot Manager=bootefi bootmgr; setenv bootmenu_1 UEFI Maintenance Menu=eficonfig"+UEFI Secure Boot Configuration +''''''''''''''''''''''''''''''
+User can enroll PK, KEK, db and dbx by selecting file.
selecting a file
OK.
Thanks, Masahisa Kojima
+"eficonfig" command only accepts the signed EFI Signature List(s) +with an authenticated header, typically ".auth" file. +To clear the PK, KEK, db and dbx, user needs to enroll the null key +signed by PK or KEK.
See also
- :doc:`bootmenu<bootmenu>` provides a simple mechanism for creating menus with different boot items
-- 2.17.1
Thanks /Ilias
participants (3)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Masahisa Kojima