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

[AKASHI Takahiro]

On Sun, Dec 02, 2018 at 11:46:29PM +0100, Alexander Graf wrote:
> 
> 
> On 12.10.18 02:55, AKASHI Takahiro wrote:
> > On Thu, Oct 11, 2018 at 04:18:33PM +0200, Heinrich Schuchardt wrote:
> >> 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.
> > 
> > I think I have already answered your questions here:
> > https://lists.denx.de/pipermail/u-boot/2018-October/343876.html
> > 
> > Did you read it?
> > 
> >> 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
> > 
> > But as far as I look at grub code (that you pointed out in your e-mail),
> > grub expects that PE headers be also "loaded" within an allocated region
> > (more specifically at the beginning of the region)
> > along with other sections in order to handle a (grub-specific? I don't know)
> > "mods" section.
> 
> Is this patch still required?

I think so as long as we want to make the code more compliant with UEFI spec,
but Heinrich may have a different opinion about how to fix it.

Thanks,
-Takahiro Akashi

> 
> Alex