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.aka...@linaro.org>
>>> Reported-by: Heinrich Schuchardt <xypron.g...@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?
Alex
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
