Re: [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values

On Thu, Mar 21, 2019 at 09:24:46PM +0100, Heinrich Schuchardt wrote:

On 10/11/18 1:11 PM, AKASHI Takahiro wrote:

...

Currently, image's image_base points to an address where the image was temporarily uploaded for further loading. Since efi_loader relocates the image to final destination, image_base and image_size should reflect that.

This bug was detected in UEFI SCT, "Loaded Image Protocol Test - test 2," which shows that 'Unload' function doesn't fit into a range suggested by image_base and image_size. TestCase/UEFI/EFI/Protocol/LoadedImage/BlackBoxTest/ LoadedImageBBTestMain.c:1002

Changes in this patch also includes:

• reverts a patch, "efi_loader: save image relocation address and size" since newly added fields are no longer needed.
• copy PE headers as well since those information will be needed for module loading, in particular, at gurb. (This bug was reported by Heinrich.)

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reported-by: Heinrich Schuchardt xypron.glpk@gmx.de

---

lib/efi_loader/efi_image_loader.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index a18ce0a5705e..d1b6c0d3cdf2 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -59,10 +59,10 @@ static efi_status_t efi_print_image_info(struct efi_loaded_image_obj *obj, { printf("UEFI image"); printf(" [0x%p:0x%p]",

•       obj->reloc_base, obj->reloc_base + obj->reloc_size - 1);
  

• if (pc && pc >= obj->reloc_base &&

•    pc < obj->reloc_base + obj->reloc_size)
  

• printf(" pc=0x%zx", pc - obj->reloc_base);
  

•       image->image_base, image->image_base + image->image_size - 1);
  

• if (pc && pc >= image->image_base &&

•    pc < image->image_base + image->image_size)
  

• printf(" pc=0x%zx", pc - image->image_base);
  

  if (image->file_path) printf(" '%pD'", image->file_path); printf("\n");

@@ -212,7 +212,6 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, int rel_idx = IMAGE_DIRECTORY_ENTRY_BASERELOC; void *entry; uint64_t image_base;

• uint64_t image_size; unsigned long virt_size = 0; int supported = 0;

@@ -256,7 +255,6 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, IMAGE_NT_HEADERS64 *nt64 = (void *)nt; IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader; image_base = opt->ImageBase;

• image_size = opt->SizeOfImage;
  

  efi_set_code_and_data_type(loaded_image_info, opt->Subsystem); efi_reloc = efi_alloc(virt_size, loaded_image_info->image_code_type);

@@ -272,7 +270,6 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, } else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader; image_base = opt->ImageBase;

• image_size = opt->SizeOfImage;
  

  efi_set_code_and_data_type(loaded_image_info, opt->Subsystem); efi_reloc = efi_alloc(virt_size, loaded_image_info->image_code_type);

@@ -291,6 +288,11 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, return NULL; }

• /* Copy PE headers */
• memcpy(efi_reloc, efi, sizeof(*dos) + sizeof(*nt)

• 	+ nt->FileHeader.SizeOfOptionalHeader
  

• 	+ num_sections * sizeof(IMAGE_SECTION_HEADER));
  

Why is it needed to copy the header? Did you experience any error related to this header not copied?

You made the same question before, and I answered already. https://lists.denx.de/pipermail/u-boot/2018-October/344165.html

-Takahiro Akashi

Best regards

Heinrich

...

/* Load sections into RAM */ for (i = num_sections - 1; i >= 0; i--) { IMAGE_SECTION_HEADER *sec = &sections[i]; @@ -315,10 +317,8 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, invalidate_icache_all();

/* Populate the loaded image interface bits */

• loaded_image_info->image_base = efi;
• loaded_image_info->image_size = image_size;
• handle->reloc_base = efi_reloc;
• handle->reloc_size = virt_size;

• loaded_image_info->image_base = efi_reloc;

• loaded_image_info->image_size = virt_size;

  return entry;

}