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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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

## 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;
  coff_header = &(header->coff_header);
  sections
    = (struct grub_pe32_section_table *) ((char *) coff_header
                                          + sizeof (*coff_header)
                                          +
coff_header->optional_header_size);

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