From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Wed, 10 Oct 2018 10:42:04 +0900 Subject: [U-Boot] [PATCH 1/1] efi_loader: set image_base and image_size to correct values In-Reply-To: References: <20180930052601.7173-1-xypron.glpk@gmx.de> <20181009065558.GP32578@linaro.org> Message-ID: <20181010014202.GW32578@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de # This discussion should be in ML as more people can join? On Tue, Oct 09, 2018 at 07:25:47PM +0200, Heinrich Schuchardt wrote: > On 10/09/2018 08:55 AM, AKASHI Takahiro wrote: > > 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 > >>> > >>> 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 > >>> > >>> 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 > >>> --- > >>> 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 I think we should carefully use those words like "load" and "relocate". > No, we should not copy more but less. The portable executable protocol > is defines that an executable can be executed without relocation if > loaded at the preferred address. Right if you mean optional header's "ImageBase" by preferred address. > This implies that the relative position > of all sections can remain unchanged Not 'can', but 'must'. This is part of "load" operation, using "VirtualAddress" in section headers. That is why we allocate a permanent space of 'virt_size' for this image. > and we can do the relocation in > place (w/o copying again). If you mean moving sections by "relocation", it's not true. What Alex's efi_loader_relocate() does is that all the slots listed in optional header's relocation information (data directory) be updated depending on a loaded address (that is, the first address of allocated space). > This is also what EDK II does. > > So when LoadImage() is called for a file load it What is passed to efi_load_pe() is just a temporary buffer > and relocate in place. and we have to load it to the final destination and relocate it (or more precisely recalculate addresses/offsets) as I said above. Thanks, -Takahiro Akashi > If LoadImage() is called for a memory location, copy it and relocate in > place. > > Best regards > > Heinrich > > > > > > >> 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 > >> > >> > >> > > >