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

[Heinrich Schuchardt]

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 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 is it needed to copy the header? Did you experience any error
related to this header not copied?

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