[PATCH v13 1/9] eficonfig: menu-driven addition of UEFI boot option

[Masahisa Kojima]

On Wed, 24 Aug 2022 at 20:15, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Kojima-san,
>
> [...]
>
> > + * eficonfig_destroy() - destroy efimenu
> > + *
> > + * @efi_menu:        pointer to the efimenu structure
> > + * @flag:    flag to free the allocated data
> > + */
> > +static void eficonfig_destroy(struct efimenu *efi_menu, bool flag)
> > +{
> > +     struct list_head *pos, *n;
> > +     struct eficonfig_entry *entry;
> > +
> > +     list_for_each_safe(pos, n, &efi_menu->list) {
> > +             entry = list_entry(pos, struct eficonfig_entry, list);
> > +             free(entry->title);
> > +             if (flag)
> > +                     free(entry->data);
>
> I don't we need this flag.  entry->data is either set to a valid pointer or
> NULL on append_entry().

There is a case that entry->data is still used after the menu is destroyed.

The following case, file_info is still used after eficonfig_destroy().
https://git.linaro.org/people/masahisa.kojima/u-boot.git/tree/cmd/eficonfig.c?h=kojima/eficonfig_upstream_v13#n953

Also there are the cases that a not-allocated pointer is set to entry->data.

In addition, this flag will be used to handle the following static menu entry
for secure boot key management series.

+       {"PK", eficonfig_process_set_secure_boot_pk, u"PK"},
+       {"KEK", eficonfig_process_set_secure_boot_key, u"KEK"},
+       {"db", eficonfig_process_set_secure_boot_key, u"db"},
+       {"dbx", eficonfig_process_set_secure_boot_key, u"dbx"},
+       {"Quit", eficonfig_process_quit},

The last parameter(u"PK", u"KEK") is set to entry->data, it will not be freed.

But this flag is a little tricky.

Thanks,
Masahisa Kojima





>
> > +             list_del(&entry->list);
> > +             free(entry);
> > +     }
> > +     free(efi_menu->menu_header);
> > +     free(efi_menu);
>
> [...]
>
> Regards
> /Ilias