Re: [PATCH v4 09/11] efi_loader: add menu-driven UEFI Boot Variable maintenance

[Masahisa Kojima <masahisa.kojima@linaro.org>]

Hi Ilias,

On Thu, 31 Mar 2022 at 17:31, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Kojima-san,
> On Thu, Mar 24, 2022 at 10:54:41PM +0900, Masahisa Kojima wrote:
> > +
>
> I haven't been able to get the patch working yet.  I'll send more feedback
> once I do.  Here's a few comments I have
>
> [...]
>
> > +struct efi_bootmenu_file_entry_data {
> > +     struct efi_bootmenu_boot_option *bo;
> > +     struct efi_file_info *f;
> > +};
> > +
> > +static efi_status_t efi_bootmenu_process_boot_selected(void *data, bool *exit);
> > +static efi_status_t efi_bootmenu_process_add_boot_option(void *data, bool *exit);
> > +static efi_status_t efi_bootmenu_process_delete_boot_option(void *data, bool *exit);
> > +static efi_status_t efi_bootmenu_process_change_boot_order(void *data, bool *exit);
>
> I think you can re-arrange some of the functions and get rid of the forward
> declarations
>
> > +
> > +static struct efi_bootmenu_item maintenance_menu_items[] = {
>
> const ?
>
> > +     {u"Add Boot Option", efi_bootmenu_process_add_boot_option},
> > +     {u"Delete Boot Option", efi_bootmenu_process_delete_boot_option},
> > +     {u"Change Boot Order", efi_bootmenu_process_change_boot_order},
> > +     {u"Quit", NULL},
> > +};
> > +
> > +static void efi_bootmenu_print_entry(void *data)
> > +{
> > +     struct efi_bootmenu_entry *entry = data;
>
> [...]
>
> > +             new_len = u16_strlen(info->bo->current_path) +
> > +                     /* TODO: show error notification to user */
> > +                     log_err("file path is too long\n");
> > +                     return EFI_INVALID_PARAMETER;
>
> Can we just check for new_len + 1 here and get rid of the follow up check ?

Sorry for the late reply.
If the selected entry is a file, not the directory, no u'\\' will be added.
Strictly speaking, checking for new_len + 1 here ends up that file path name
max size is (EFI_BOOTMENU_FILE_PATH_MAX -1).

>
> > +             }
> > +             u16_strlcat(info->bo->current_path, info->f->file_name, EFI_BOOTMENU_FILE_PATH_MAX);
> > +             if (info->f->attribute & EFI_FILE_DIRECTORY) {
> > +                     if (new_len + 1 >= EFI_BOOTMENU_FILE_PATH_MAX) {
> > +                             log_err("file path is too long\n");
> > +                             return EFI_INVALID_PARAMETER;
> > +                     }
> > +                     u16_strlcat(info->bo->current_path, u"\\", EFI_BOOTMENU_FILE_PATH_MAX);
> > +             } else {
> > +                     info->bo->file_selected = true;
> > +             }
>
> [...]
>
> > +             menu_item = calloc(count + 1, sizeof(struct efi_bootmenu_item));
> > +             if (!menu_item) {
> > +                     efi_file_close_int(f);
> > +                     free(dir_buf);
> > +                     ret = EFI_OUT_OF_RESOURCES;
> > +                     goto out;
> > +             }
> > +
> > +             /* read directory and construct menu structure */
> > +             efi_file_setpos_int(f, 0);
> > +             iter = menu_item;
> > +             ptr = (struct efi_file_info *)dir_buf;
>
> This will cause an unaligned access later on when you access
> ptr->attribute.  Any reason we can't allocate ptr directly?

I will fix other comments.

Thanks,
Masahisa Kojima


>
> > +             for (i = 0; i < count; i++) {
> > +                     int name_len;
> > +                     u16 *name;
> > +                     struct efi_bootmenu_file_entry_data *info;
> > +
> > +                     len = size;
> > +                     ret = efi_file_read_int(f, &len, ptr);
> > +                     if (ret != EFI_SUCCESS || len == 0)
> > +                             goto err;
> > +
> > +                     if (ptr->attribute & EFI_FILE_DIRECTORY) {
> > +                             /* append u'/' at the end of directory name */
> > +                             name_len = u16_strsize(ptr->file_name) + sizeof(u16);
> > +                             name = calloc(1, name_len);
> > +                             if (!name) {
> > +                                     ret = EFI_OUT_OF_RESOURCES;
> > +                                     goto err;
> > +                             }
> > +                             u16_strcpy(name, ptr->file_name);
> > +                             name[u16_strlen(ptr->file_name)] = u'/';
> > +                     } else {
> > +                             name_len = u16_strsize(ptr->file_name);
> > +                             name = calloc(1, name_len);
> > +                             if (!name) {
> > +                                     ret = EFI_OUT_OF_RESOURCES;
> > +                                     goto err;
> > +                             }
> > +                             u16_strcpy(name, ptr->file_name);
> > +                     }
> > +
> > +                     info = calloc(1, sizeof(struct efi_bootmenu_file_entry_data));
> > +                     if (!info) {
> > +                             ret = EFI_OUT_OF_RESOURCES;
> > +                             goto err;
> > +                     }
> > +                     info->f = ptr;
> > +                     info->bo = bo;
> > +                     iter->title = name;
> > +                     iter->func = efi_bootmenu_file_selected;
> > +                     iter->data = info;
> > +                     iter++;
> > +
> > +                     size -= len;
> > +                     ptr = (struct efi_file_info *)((char *)ptr + len);
>
> ditto
>
> > +             }
> > +
> > +             /* add "Quit" entry */
> > +             iter->title = u"Quit";
> > +             iter->func = NULL;
> > +             iter->data = NULL;
> > +             count += 1;
> > +
> > +             ret = efi_bootmenu_process_common(menu_item, count, -1);
> > +err:
> > +             efi_file_close_int(f);
> > +             iter = menu_item;
> > +             for (i = 0; i < count - 1; i++, iter++) {
> > +                     free(iter->title);
> > +                     free(iter->data);
> > +             }
> > +
> > +             free(dir_buf);
> > +             free(menu_item);
> > +
> > +             if (ret != EFI_SUCCESS)
> > +                     break;
> > +     }
> > +
> > +out:
> > +     free(buf);
> > +     return ret;
> > +}
> > +
>
> [...]
>
> Regards
> /Ilias