[PATCH 2/2] eficonfig: avoid SetVariable between GetNextVariableName calls

[Masahisa Kojima]

On Fri, 16 Dec 2022 at 10:58, Heinrich Schuchardt <xypron.glpk at 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 at 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;
> >   }
>