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

[Heinrich Schuchardt]

On 10/11/2018 01: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 at linaro.org>
> Reported-by: Heinrich Schuchardt <xypron.glpk at 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 do we have to copy PE headers and the sections below separately? My
understanding is that the relative positions do not need any adjustment.

Nothing in the spec requires the COFF header to be at offset
sizeof(dos). You can put the COFF headder anywhere in the file. Please,
read

https://media.blackhat.com/bh-us-11/Vuksan/BH_US_11_VuksanPericin_PECOFF_WP.pdf

We should not assign new memory here. Nor should we copy anything here.
We should simply use the relocation table to update the indicated memory
locations.

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;
>  }
>