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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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