[U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> In the current implementation, bootefi command and EFI boot manager
> don't use load_image API, instead, use more primitive and internal
> functions. This will introduce duplicated code and potentially
> unknown bugs as well as inconsistent behaviours.
>
> With this patch, do_efibootmgr() and do_boot_efi() are completely
> overhauled and re-implemented using load_image API.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/bootefi.c                 | 197 +++++++++++++++++++---------------
>   include/efi_loader.h          |   5 +-
>   lib/efi_loader/efi_bootmgr.c  |  43 ++++----
>   lib/efi_loader/efi_boottime.c |   2 +
>   4 files changed, 134 insertions(+), 113 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index f14966961b23..97d49a53a44b 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt)
>   /**
>    * do_bootefi_exec() - execute EFI binary
>    *
> - * @efi:		address of the binary
> - * @device_path:	path of the device from which the binary was loaded
> - * @image_path:		device path of the binary
> + * @handle:		handle of loaded image
>    * Return:		status code
>    *
>    * Load the EFI binary into a newly assigned memory unwinding the relocation
>    * information, install the loaded image protocol, and call the binary.
>    */
> -static efi_status_t do_bootefi_exec(void *efi,
> -				    struct efi_device_path *device_path,
> -				    struct efi_device_path *image_path)
> +static efi_status_t do_bootefi_exec(efi_handle_t handle)
>   {
> -	efi_handle_t mem_handle = NULL;
> -	struct efi_device_path *memdp = NULL;
> -	efi_status_t ret;
> -	struct efi_loaded_image_obj *image_obj = NULL;
>   	struct efi_loaded_image *loaded_image_info = NULL;
> -
> -	/*
> -	 * Special case for efi payload not loaded from disk, such as
> -	 * 'bootefi hello' or for example payload loaded directly into
> -	 * memory via JTAG, etc:
> -	 */
> -	if (!device_path && !image_path) {
> -		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> -		/* actual addresses filled in after efi_load_pe() */
> -		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
> -		device_path = image_path = memdp;
> -		/*
> -		 * Grub expects that the device path of the loaded image is
> -		 * installed on a handle.
> -		 */
> -		ret = efi_create_handle(&mem_handle);
> -		if (ret != EFI_SUCCESS)
> -			return ret; /* TODO: leaks device_path */
> -		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> -				       device_path);
> -		if (ret != EFI_SUCCESS)
> -			goto err_add_protocol;
> -	} else {
> -		assert(device_path && image_path);
> -	}
> -
> -	ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> -				     &loaded_image_info);
> -	if (ret)
> -		goto err_prepare;
> +	efi_status_t ret;
>
>   	/* Transfer environment variable as load options */
> -	set_load_options(loaded_image_info, "bootargs");

In set_load_options() an error could occur (out of memory). So I think
it should return a status.

> -
> -	/* Load the EFI payload */
> -	ret = efi_load_pe(image_obj, efi, loaded_image_info);
> +	ret = EFI_CALL(systab.boottime->open_protocol(
> +					handle,
> +					&efi_guid_loaded_image,
> +					(void **)&loaded_image_info,
> +					NULL, NULL,
> +					EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));

Shouldn't we move this to set_load_options()?

>   	if (ret != EFI_SUCCESS)
> -		goto err_prepare;
> -
> -	if (memdp) {
> -		struct efi_device_path_memory *mdp = (void *)memdp;
> -		mdp->memory_type = loaded_image_info->image_code_type;
> -		mdp->start_address = (uintptr_t)loaded_image_info->image_base;
> -		mdp->end_address = mdp->start_address +
> -				loaded_image_info->image_size;
> -	}
> +		goto err;
> +	set_load_options(loaded_image_info, "bootargs");
>
>   	/* we don't support much: */
>   	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>   		"{ro,boot}(blob)0000000000000000");

Shouldn't this move to efi_setup.c? Probably we should also set
OsIndications. I would prefer using efi_set_variable(). I think moving
this could be done in a preparatory patch.

>
>   	/* Call our payload! */
> -	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> -	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> +	ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
>
> -err_prepare:
> -	/* image has returned, loaded-image obj goes *poof*: */
>   	efi_restore_gd();
>   	free(loaded_image_info->load_options);
> -	efi_delete_handle(&image_obj->header);
> -
> -err_add_protocol:
> -	if (mem_handle)
> -		efi_delete_handle(mem_handle);
>
> +err:
>   	return ret;
>   }
>
> @@ -317,8 +268,7 @@ err_add_protocol:
>    */
>   static int do_efibootmgr(const char *fdt_opt)
>   {
> -	struct efi_device_path *device_path, *file_path;
> -	void *addr;
> +	efi_handle_t handle;
>   	efi_status_t ret;
>
>   	/* Allow unaligned memory access */
> @@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
>   	else if (ret != EFI_SUCCESS)
>   		return CMD_RET_FAILURE;
>
> -	addr = efi_bootmgr_load(&device_path, &file_path);
> -	if (!addr)
> -		return 1;
> +	ret = efi_bootmgr_load(&handle);
> +	if (ret != EFI_SUCCESS) {
> +		printf("EFI boot manager: Cannot load any image\n");
> +		return CMD_RET_FAILURE;
> +	}
>
> -	printf("## Starting EFI application at %p ...\n", addr);
> -	ret = do_bootefi_exec(addr, device_path, file_path);
> -	printf("## Application terminated, r = %lu\n",
> -	       ret & ~EFI_ERROR_MASK);
> +	ret = do_bootefi_exec(handle);
> +	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
>
>   	if (ret != EFI_SUCCESS)
> -		return 1;
> +		return CMD_RET_FAILURE;
>
> -	return 0;
> +	return CMD_RET_SUCCESS;
>   }
>
>   /*
> @@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt)
>    */
>   static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>   {
> -	unsigned long addr;
> -	char *saddr;
> +	void *image_buf;
> +	struct efi_device_path *device_path, *image_path;
> +	struct efi_device_path *memdp = NULL, *file_path = NULL;
> +	const char *saddr;
> +	unsigned long addr, size;
> +	const char *size_str;
> +	efi_handle_t mem_handle = NULL, handle;
>   	efi_status_t ret;
> -	unsigned long fdt_addr;
>
>   	/* Allow unaligned memory access */
>   	allow_unaligned();
> @@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>   		return CMD_RET_FAILURE;
>
>   #ifdef CONFIG_CMD_BOOTEFI_HELLO
> -	if (!strcmp(argv[1], "hello")) {
> -		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> -
> +	if (!strcmp(image_opt, "hello")) {
>   		saddr = env_get("loadaddr");
> +		size = __efi_helloworld_end - __efi_helloworld_begin;
> +
>   		if (saddr)
>   			addr = simple_strtoul(saddr, NULL, 16);
>   		else
>   			addr = CONFIG_SYS_LOAD_ADDR;
> -		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> +
> +		image_buf = map_sysmem(addr, size);
> +		memcpy(image_buf, __efi_helloworld_begin, size);
> +
> +		device_path = NULL;
> +		image_path = NULL;
>   	} else
>   #endif
>   	{
> -		saddr = argv[1];
> +		saddr = image_opt;
> +		size_str = env_get("filesize");
> +		if (size_str)
> +			size = simple_strtoul(size_str, NULL, 16);
> +		else
> +			size = 0;
>
>   		addr = simple_strtoul(saddr, NULL, 16);
>   		/* Check that a numeric value was passed */
>   		if (!addr && *saddr != '0')
>   			return CMD_RET_USAGE;
> +
> +		image_buf = map_sysmem(addr, size);
> +
> +		device_path = bootefi_device_path;
> +		image_path = bootefi_image_path;
>   	}
>
> -	printf("## Starting EFI application at %08lx ...\n", addr);
> -	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> -			      bootefi_image_path);
> -	printf("## Application terminated, r = %lu\n",
> -	       ret & ~EFI_ERROR_MASK);
> +	if (!device_path && !image_path) {
> +		/*
> +		 * Special case for efi payload not loaded from disk,
> +		 * such as 'bootefi hello' or for example payload
> +		 * loaded directly into memory via JTAG, etc:
> +		 */
> +		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");

The EFI spec says that either of SourceBuffer or DevicePath may be NULL
when calling LoadImage().

> +
> +		/* actual addresses filled in after efi_load_image() */
> +		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +					(uint64_t)image_buf, size);
> +		file_path = memdp; /* append(memdp, NULL) */
> +
> +		/*
> +		 * Make sure that device for device_path exist
> +		 * in load_image(). Otherwise, shell and grub will fail.

GRUB will fail anyway because it cannot determine the disk from which it
was loaded. So why are we doing this?

> +		 */
> +		ret = efi_create_handle(&mem_handle);
> +		if (ret != EFI_SUCCESS)
> +			/* TODO: leaks device_path */
> +			goto err_add_protocol;
> +
> +		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> +				       memdp);

Couldn't we as well use the device path of the root node as "file_path"?

> +		if (ret != EFI_SUCCESS)
> +			goto err_add_protocol;
> +	} else {
> +		assert(device_path && image_path);
> +		file_path = efi_dp_append(device_path, image_path);

Where is file_path freed?

> +	}
> +
> +	ret = EFI_CALL(efi_load_image(false, efi_root,
> +				      file_path, image_buf, size, &handle));
> +	if (ret != EFI_SUCCESS)
> +		goto err_prepare;
> +
> +	ret = do_bootefi_exec(handle);
> +	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> +
> +err_prepare:
> +	if (device_path)
> +		efi_free_pool(file_path);
> +
> +err_add_protocol:
> +	if (mem_handle)
> +		efi_delete_handle(mem_handle);
> +	if (memdp)
> +		efi_free_pool(memdp);
>
>   	if (ret != EFI_SUCCESS)
>   		return CMD_RET_FAILURE;
> -	else
> -		return CMD_RET_SUCCESS;
> +
> +	return CMD_RET_SUCCESS;
>   }
>
>   #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> @@ -581,7 +593,7 @@ static char bootefi_help_text[] =
>   	"    Use environment variable efi_selftest to select a single test.\n"
>   	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
>   #endif
> -	"bootefi bootmgr [fdt addr]\n"
> +	"bootefi bootmgr [fdt address]\n"
>   	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
>   	"\n"
>   	"    If specified, the device tree located at <fdt address> gets\n"
> @@ -606,6 +618,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
>   	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
>   	if (ret == EFI_SUCCESS) {
>   		bootefi_device_path = device;
> +		if (image) {
> +			/* FIXME: image should not contain device */
> +			struct efi_device_path *image_tmp = image;
> +
> +			efi_dp_split_file_path(image, &device, &image);
> +			efi_free_pool(image_tmp);
> +		}
>   		bootefi_image_path = image;
>   	} else {
>   		bootefi_device_path = NULL;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index d524dc7e24c1..c3eda2bb05d7 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -408,8 +408,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>   				    struct efi_device_path *file_path,
>   				    struct efi_loaded_image_obj **handle_ptr,
>   				    struct efi_loaded_image **info_ptr);
> -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> -				      void **buffer, efi_uintn_t *size);
>   /* Print information about all loaded images */
>   void efi_print_image_infos(void *pc);
>
> @@ -563,8 +561,7 @@ struct efi_load_option {
>
>   void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
>   unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> -		       struct efi_device_path **file_path);
> +efi_status_t efi_bootmgr_load(efi_handle_t *handle);
>
>   #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 4fccadc5483d..13ec79b2098b 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,
>    * if successful.  This checks that the EFI_LOAD_OPTION is active (enabled)
>    * and that the specified file to boot exists.
>    */
> -static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> -			    struct efi_device_path **file_path)
> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
>   {
>   	struct efi_load_option lo;
>   	u16 varname[] = L"Boot0000";
>   	u16 hexmap[] = L"0123456789ABCDEF";
> -	void *load_option, *image = NULL;
> +	void *load_option;
>   	efi_uintn_t size;
> +	efi_status_t ret;
>
>   	varname[4] = hexmap[(n & 0xf000) >> 12];
>   	varname[5] = hexmap[(n & 0x0f00) >> 8];
> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>
>   	load_option = get_var(varname, &efi_global_variable_guid, &size);
>   	if (!load_option)
> -		return NULL;
> +		return EFI_LOAD_ERROR;
>
>   	efi_deserialize_load_option(&lo, load_option);
>
>   	if (lo.attributes & LOAD_OPTION_ACTIVE) {
>   		u32 attributes;
> -		efi_status_t ret;
>
>   		debug("%s: trying to load \"%ls\" from %pD\n",
>   		      __func__, lo.label, lo.file_path);
>
> -		ret = efi_load_image_from_path(lo.file_path, &image, &size);
> -
> +		ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> +					      lo.file_path, NULL, 0,
> +					      handle));
>   		if (ret != EFI_SUCCESS)
>   			goto error;
>
> @@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>   				L"BootCurrent",
>   				(efi_guid_t *)&efi_global_variable_guid,
>   				attributes, size, &n));
> -		if (ret != EFI_SUCCESS)
> +		if (ret != EFI_SUCCESS) {
> +			if (EFI_CALL(efi_unload_image(*handle))
> +			    != EFI_SUCCESS)
> +				printf("Unloading image failed\n");
>   			goto error;
> +		}
>
>   		printf("Booting: %ls\n", lo.label);
> -		efi_dp_split_file_path(lo.file_path, device_path, file_path);
> +	} else {
> +		ret = EFI_LOAD_ERROR;
>   	}
>
>   error:
>   	free(load_option);
>
> -	return image;
> +	return ret;
>   }
>
>   /*
> @@ -177,12 +182,10 @@ error:
>    * EFI variable, the available load-options, finding and returning
>    * the first one that can be loaded successfully.
>    */
> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> -		       struct efi_device_path **file_path)
> +efi_status_t efi_bootmgr_load(efi_handle_t *handle)
>   {
>   	u16 bootnext, *bootorder;
>   	efi_uintn_t size;
> -	void *image = NULL;
>   	int i, num;
>   	efi_status_t ret;
>
> @@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
>   		/* load BootNext */
>   		if (ret == EFI_SUCCESS) {
>   			if (size == sizeof(u16)) {
> -				image = try_load_entry(bootnext, device_path,
> -						       file_path);
> -				if (image)
> -					return image;
> +				ret = try_load_entry(bootnext, handle);
> +				if (ret == EFI_SUCCESS)
> +					return ret;
>   			}
>   		} else {
>   			printf("Deleting BootNext failed\n");
> @@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
>   	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
>   	if (!bootorder) {
>   		printf("BootOrder not defined\n");
> +		ret = EFI_NOT_FOUND;
>   		goto error;
>   	}
>
>   	num = size / sizeof(uint16_t);
>   	for (i = 0; i < num; i++) {
>   		debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
> -		image = try_load_entry(bootorder[i], device_path, file_path);
> -		if (image)
> +		ret = try_load_entry(bootorder[i], handle);
> +		if (ret == EFI_SUCCESS)
>   			break;
>   	}
>
>   	free(bootorder);
>
>   error:
> -	return image;
> +	return ret;
>   }
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index b215bd7723da..65425fabc08f 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1611,6 +1611,7 @@ failure:
>    * @size:	size of the loaded image
>    * Return:	status code
>    */
> +static
>   efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
>   				      void **buffer, efi_uintn_t *size)
>   {
> @@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>   	}
>
>   	current_image = image_handle;
> +	debug("EFI: Jumping into 0x%p\n", image_obj->entry);

Please, use EFI_PRINT() here.

Best regards

Heinrich

>   	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>
>   	/*
>