[U-Boot] [PATCH 1/1] efi_loader: set image_base and image_size to correct values

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

# This discussion should be in ML as more people can join?

On Tue, Oct 09, 2018 at 07:25:47PM +0200, Heinrich Schuchardt wrote:
> On 10/09/2018 08:55 AM, AKASHI Takahiro wrote:
> > On Sat, Oct 06, 2018 at 09:51:01AM +0200, Heinrich Schuchardt wrote:
> >> On 09/30/2018 07:26 AM, Heinrich Schuchardt wrote:
> >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>
> >>> 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
> >>>
> >>> This patch also reverts a patch, "efi_loader: save image relocation address
> >>> and size" since newly added fields are no longer needed.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>
> >>> Rebase the patch. Keep the relocation address in struct efi_image_object.
> >>> We will use the address to free the image in UnloadImage.
> >>>
> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> ---
> >>>  lib/efi_loader/efi_image_loader.c | 7 ++-----
> >>>  1 file changed, 2 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> >>> index a18ce0a5705..39902152f3c 100644
> >>> --- a/lib/efi_loader/efi_image_loader.c
> >>> +++ b/lib/efi_loader/efi_image_loader.c
> >>> @@ -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);
> >>> @@ -315,10 +312,10 @@ 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;
> >>>  }
> >>>
> >>
> >> With this patch GRUB is not able to load the modules which are included
> >> in grubaa64.efi
> > 
> > Oh, really?
> > 
> >> ## Starting EFI application at 40400000 ...
> >> error: unknown filesystem.
> >> Entering rescue mode...
> >> grub rescue>
> >>
> >> Function grub_efi_modules_addr() expects image_base to point to the
> >> unrelocated image:
> >>
> >>   header = image->image_base;
> > 
> > Here "image" points to 'grub' itself, right?
> > If so,
> > 
> >>   coff_header = &(header->coff_header);
> >>   sections
> >>     = (struct grub_pe32_section_table *) ((char *) coff_header
> >>                               + sizeof (*coff_header)
> >>                               +
> >> coff_header->optional_header_size);
> > 
> > It seems to me that the above code shows that we should also
> > copy PE headers, along with sections, after a new location
> > is allocated in efi_load_pe().
> > 
> > Then my patch should work again, I didn't test it though.
> > 
> > Thanks,
> > -Takahiro Akashi


I think we should carefully use those words like "load" and "relocate".

> No, we should not copy more but less. The portable executable protocol
> is defines that an executable can be executed without relocation if
> loaded at the preferred address.

Right if you mean optional header's "ImageBase" by preferred address.

> This implies that the relative position
> of all sections can remain unchanged

Not 'can', but 'must'.
This is part of "load" operation, using "VirtualAddress" in section headers.
That is why we allocate a permanent space of 'virt_size' for this image.

> and we can do the relocation in
> place (w/o copying again).

If you mean moving sections by "relocation", it's not true.
What Alex's efi_loader_relocate() does is that all the slots listed
in optional header's relocation information (data directory) be
updated depending on a loaded address (that is, the first address of
allocated space).


> This is also what EDK II does.
> 
> So when LoadImage() is called for a file load it

What is passed to efi_load_pe() is just a temporary buffer

> and relocate in place.

and we have to load it to the final destination and relocate it
(or more precisely recalculate addresses/offsets) as I said above.

Thanks,
-Takahiro Akashi

> If LoadImage() is called for a memory location, copy it and relocate in
> place.
> 
> Best regards
> 
> Heinrich
> 
> > 
> > 
> >> The UEFI SCT II specifcation test 5.3.1.1.11 requires
> >>
> >> Check on Application Images which have Unload function.
> >> Unload field should be valid and its entry address should be within
> >> the range of [ImageBase, ImageBase+ImageSize]
> >>
> >> @Leif
> >> Any idea how to sort this out?
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>
> >>
> > 
>