| Message ID | 20221208134022.28621-3-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix eficonfig GetNextVariableName calls handling | expand |
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; > }
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; > > } >
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; }
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(-)