Re: [U-Boot] [PATCH 1/5] efi_loader: LoadImage: always allocate new pages

On 28.12.18 12:41, Heinrich Schuchardt wrote:

If we want to properly unload images in Exit() the memory should always be allocated in the same way. As we allocate memory when reading from file we should do the same when the original image is in memory.

Further patches will be needed:

• use LoadImage() in bootefi and bootmgr
• implement correct unloading of images in Exit()

Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de

include/efi_loader.h | 2 +- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_boottime.c | 56 ++++++++++++++++++++++++----------- 3 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 53f08161ab..a4c869b623 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -387,7 +387,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_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);
  

• 		      void **buffer, efi_uintn_t *size);
  

/* Print information about all loaded images */ void efi_print_image_infos(void *pc);

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a095df3f54..196116b547 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -150,7 +150,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, 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);
  

• ret = efi_load_image_from_path(lo.file_path, &image, &size);
  
  

  if (ret != EFI_SUCCESS) goto error;

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index f592e4083f..ba76f7ff50 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1566,17 +1566,19 @@ failure:

/**

• efi_load_image_from_path() - load an image using a file path

• • @file_path: the path of the image to load
• • @buffer: buffer containing the loaded image

• • @file_path: the path of the image to load
• • @buffer: buffer containing the loaded image
• • @size: size of the loaded image
  • Return: status code
  */

efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,

• 		      void **buffer)
  

• 		      void **buffer, efi_uintn_t *size)
  

{ struct efi_file_info *info = NULL; struct efi_file_handle *f; static efi_status_t ret;

• u64 addr; efi_uintn_t bs;

  f = efi_file_from_path(file_path);

@@ -1594,22 +1596,21 @@ efi_status_t efi_load_image_from_path(struct efi_device_path *file_path, if (ret != EFI_SUCCESS) goto error;

• ret = efi_allocate_pool(EFI_LOADER_DATA, info->file_size, buffer);
• if (ret)

• ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,

• 		 EFI_RUNTIME_SERVICES_CODE,
  

• 		 efi_size_in_pages(), &addr);
  

Doesn't efi_size_in_pages() want an argument?

Also, why is this RTS code?

• if (ret != EFI_SUCCESS) {

• ret = EFI_OUT_OF_RESOURCES;
  

  goto error;

• }

• *buffer = (void *)(uintptr_t)addr; bs = info->file_size; EFI_CALL(ret = f->read(f, &bs, *buffer));

error:

• free(info);

Why don't you have to free info anymore? It's a local variable, no?

EFI_CALL(f->close(f));

• if (ret != EFI_SUCCESS) {

• efi_free_pool(*buffer);
  

Where do we ever free the pages allocated above?

• *buffer = NULL;
  

• }
• return ret;

}

@@ -1636,6 +1637,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy, efi_uintn_t source_size, efi_handle_t *image_handle) {

• struct efi_device_path *dp, *fp; struct efi_loaded_image *info = NULL; struct efi_loaded_image_obj **image_obj = (struct efi_loaded_image_obj **)image_handle;

@@ -1655,9 +1657,10 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy, }

if (!source_buffer) {

• struct efi_device_path *dp, *fp;
  

• ret = efi_load_image_from_path(file_path, &source_buffer);
  

• ret = efi_load_image_from_path(file_path, &source_buffer,
  

• 			       &source_size);
  

• if (ret == EFI_OUT_OF_RESOURCES)
  

• 	goto error;
  

How is error different from failure? And why does it depend on the error code of this function? I must be missing something ...

if (ret != EFI_SUCCESS)
	goto failure;
/*

@@ -1665,26 +1668,43 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy, * file parts: */ efi_dp_split_file_path(file_path, &dp, &fp);

• ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
  

• if (ret != EFI_SUCCESS)
  

• 	goto failure;
  

  } else { /* In this case, file_path is the "device" path, i.e.
  • something like a HARDWARE_DEVICE:MEMORY_MAPPED
  */

• ret = efi_setup_loaded_image(file_path, NULL, image_obj, &info);
  

• u64 addr;
  

• void *dest_buffer;
  

• ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
  

• 			 EFI_RUNTIME_SERVICES_CODE,
  

• 			 efi_size_in_pages(source_size), &addr);
  

  if (ret != EFI_SUCCESS) goto error;

• dest_buffer = (void *)(uintptr_t)addr;
  

• memcpy(dest_buffer, source_buffer, source_size);
  

I'd really by far prefer if we didn't have to memcpy() things around and reserve more memory than we have to.

Can't we just instead of this create a destructor that we attach to the LoadedImage that does different things depending on the way it got loaded?

Alex

• source_buffer = dest_buffer;
  

• dp = file_path;
  

• fp = NULL;
  

  }
• ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
• if (ret != EFI_SUCCESS)

• goto failure;
  

  (*image_obj)->entry = efi_load_pe(*image_obj, source_buffer, info); if (!(*image_obj)->entry) { ret = EFI_UNSUPPORTED; goto failure; }
• /* Update the type of the allocated memory */
• efi_add_memory_map((uintptr_t)source_buffer,

• 	   efi_size_in_pages(source_size),
  

• 	   info->image_code_type, false);
  

  info->system_table = &systab; info->parent_handle = parent_image; return EFI_EXIT(EFI_SUCCESS);

failure:

• efi_free_pages((uintptr_t)source_buffer,

•        efi_size_in_pages(source_size));
  

  efi_delete_handle(*image_handle); *image_handle = NULL; free(info);