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

[AKASHI Takahiro]

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 at 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 at 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 at 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


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