From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Mon, 3 Dec 2018 17:26:02 +0900 Subject: [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values In-Reply-To: References: <20181011111140.29665-1-takahiro.akashi@linaro.org> <65aca52b-ddc4-8457-56cf-e20a15c231dc@gmx.de> <20181012005500.GZ32578@linaro.org> Message-ID: <20181203082601.GI28995@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 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 > >>> Reported-by: Heinrich Schuchardt > >>> --- > >>> 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