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

Correction:

On Thu, Apr 18, 2019 at 09:13:52AM +0900, AKASHI Takahiro wrote:

On Tue, Apr 16, 2019 at 12:56:32PM +0200, Heinrich Schuchardt wrote:

...

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.

I didn't change anything in set_load_options(), but I will follow your comment.

...

...

• /* 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()?

No. This line has nothing to do with "load options."

Wrong, I will follow your comment. This change, however, uncovers one issue: Who is responsible for freeing loaded_image_info->load_option, particularly, when efi_exit()/unload_image() is fully implemented? In such a case, we have no chance to access this handle, hence loaded_image_info, after returning from efi_start_image().

Thanks, -Takahiro Akashi

...

...

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.

Yeah, I am aware of this issue. Will fix in a separate 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().

So are you suggesting we should remove this message?

...

...

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

I will comment on another reply.

...

...

•  */
  

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

I don't get you point very well, but it will depend on how we should fix a grub issue mentioned above.

...

...

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

In this function. Will fix.

...

...

• }
• 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.

Basically I agree, but there are lots of debug()'s in this file. We should replace them all at once later.

Thanks, -Takahiro Akashi

...

Best regards

Heinrich

...

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

/*