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

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 ?

• }
  

• 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?

• 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