Re: [U-Boot] [RFC v2 10/11] efi_loader: rework bootmgr/bootefi using load_image API

On Fri, Apr 12, 2019 at 07:53:25AM +0200, Heinrich Schuchardt wrote:

On 3/27/19 5:40 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 | 199 +++++++++++++++++++--------------- include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 43 ++++---- lib/efi_loader/efi_boottime.c | 1 + 4 files changed, 138 insertions(+), 110 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 39a0712af6ad..17dccd718008 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -224,88 +224,43 @@ 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");
• /* 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));
  

  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");

  /* Call our payload! */

• image_obj = container_of(handle, struct efi_loaded_image_obj, header);

This is not needed, if we remove the debug statement.

...

debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);

That line should go to StartImage() if needed. Please, drop it here.

...

• 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);
  

• efi_free_pool(loaded_image_info);

Wouldn't this be done by unload_image()?

but we should not call unload_image(), efi_exit() does. Right?

...

+err: return ret; }

@@ -319,8 +274,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 +294,21 @@ static int do_efibootmgr(const char *fdt_opt) 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);
• EFI_CALL(efi_unload_image(handle));

Only applications have to be unloaded, not drivers. Unloading is to be handled by exit(), see UEFI spec.

How do we know whether the loaded image is an application, or driver?

...

if (ret != EFI_SUCCESS)

• return 1;
  

• return CMD_RET_FAILURE;
  
  

• return 0;

• return CMD_RET_SUCCESS;

}

/* @@ -367,10 +323,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();

@@ -390,36 +350,96 @@ 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;
  

• }

• 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");
  

• /* 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.
  

•  */
  

• 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);
  

• if (ret != EFI_SUCCESS)
  

• 	goto err_add_protocol;
  

• } else {

• assert(device_path && image_path);
  

• file_path = efi_dp_append(device_path, image_path);
  

  }

• ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,

• 		      file_path, image_buf, size, &handle));
  

• if (ret != EFI_SUCCESS)

• goto err_prepare;
  

• 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);
  

• ret = do_bootefi_exec(handle);
• printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);

Why should we need all this duplicate code. Cant we move that to do_bootefi_exec()?

...

• EFI_CALL(efi_unload_image(handle));

That is the task of efi_exit().

Who should be blamed for reclaiming a handle which is created with load_image()? What if an application doesn't call efi_exit() by itself?

-Takahiro Akashi

Best regards

Heinrich

...

+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 @@ -577,7 +597,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"

@@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -410,8 +410,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);

@@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1610,6 +1610,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) {