On Thu, Mar 21, 2019 at 09:24:46PM +0100, Heinrich Schuchardt wrote:
> 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 <[email protected]>
> > Reported-by: Heinrich Schuchardt <[email protected]>
> > ---
> >  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?

You made the same question before, and I answered already.
https://lists.denx.de/pipermail/u-boot/2018-October/344165.html

-Takahiro Akashi

> 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;
> >  }
> >
> 
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot

Reply via email to