| Message ID | 20221219023314.23959-3-masahisa.kojima@linaro.org |
|---|---|
| State | Accepted |
| Commit | 645502743ac10ffaa5abf944bf0b751b32b47220 |
| Headers | show |
| Series | fix eficonfig GetNextVariableName calls handling | expand |
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 Kojima<masahisa.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; > + } > if (ret != EFI_SUCCESS) > goto out; > > @@ -2349,31 +2356,46 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op > if (ret != EFI_SUCCESS) > goto next; > > - if (size >= sizeof(efi_guid_bootmenu_auto_generated)) { > - if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) { > - for (i = 0; i < count; i++) { > - if (opt[i].size == tmp && > - memcmp(opt[i].lo, load_option, tmp) == 0) { > - opt[i].exist = true; > - break; > - } > + if (size >= sizeof(efi_guid_bootmenu_auto_generated) && > + !guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) { > + for (i = 0; i < count; i++) { > + if (opt[i].size == tmp && > + memcmp(opt[i].lo, load_option, tmp) == 0) { > + opt[i].exist = true; > + break; > } > + } > > - if (i == count) { > - ret = delete_boot_option(i); > - if (ret != EFI_SUCCESS) { > - free(load_option); > - goto out; > - } > + /* > + * The entire list of variables must be retrieved by > + * efi_get_next_variable_name_int() before deleting the invalid > + * boot option, just save the index here. > + */ > + if (i == count) { > + p = realloc(delete_index_list, sizeof(u32) * > + (list_size + 1)); > + if (!p) { > + ret = EFI_OUT_OF_RESOURCES; > + goto out; > } > + delete_index_list = p; > + delete_index_list[list_size++] = index; > } > } > next: > free(load_option); > } > > + /* delete all invalid boot options */ > + for (i = 0; i < list_size; i++) { > + ret = delete_boot_option(delete_index_list[i]); > + if (ret != EFI_SUCCESS) > + goto out; > + } > + > out: > free(var_name16); > + free(delete_index_list); > > return ret; > } > -- > 2.17.1 > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
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; }
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(-)