* [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values
@ 2018-10-11 11:11 AKASHI Takahiro
2018-10-11 14:18 ` Heinrich Schuchardt
2019-03-21 20:24 ` Heinrich Schuchardt
0 siblings, 2 replies; 8+ messages in thread
From: AKASHI Takahiro @ 2018-10-11 11:11 UTC (permalink / raw)
To: u-boot
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.akashi@linaro.org>
Reported-by: Heinrich Schuchardt <xypron.glpk@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));
+
/* Load sections into RAM */
for (i = num_sections - 1; i >= 0; i--) {
IMAGE_SECTION_HEADER *sec = §ions[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;
}
--
2.19.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values
2018-10-11 11:11 [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values AKASHI Takahiro
@ 2018-10-11 14:18 ` Heinrich Schuchardt
2018-10-12 0:55 ` AKASHI Takahiro
2019-03-21 20:24 ` Heinrich Schuchardt
1 sibling, 1 reply; 8+ messages in thread
From: Heinrich Schuchardt @ 2018-10-11 14:18 UTC (permalink / raw)
To: u-boot
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.akashi@linaro.org>
> Reported-by: Heinrich Schuchardt <xypron.glpk@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.
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
https://media.blackhat.com/bh-us-11/Vuksan/BH_US_11_VuksanPericin_PECOFF_WP.pdf
We should not assign new memory here. Nor should we copy anything here.
We should simply use the relocation table to update the indicated memory
locations.
Best regards
Heinrich
> /* Load sections into RAM */
> for (i = num_sections - 1; i >= 0; i--) {
> IMAGE_SECTION_HEADER *sec = §ions[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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values
2018-10-11 14:18 ` Heinrich Schuchardt
@ 2018-10-12 0:55 ` AKASHI Takahiro
2018-12-02 22:46 ` Alexander Graf
0 siblings, 1 reply; 8+ messages in thread
From: AKASHI Takahiro @ 2018-10-12 0:55 UTC (permalink / raw)
To: u-boot
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.akashi@linaro.org>
> > Reported-by: Heinrich Schuchardt <xypron.glpk@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.
-Takahiro Akashi
> https://media.blackhat.com/bh-us-11/Vuksan/BH_US_11_VuksanPericin_PECOFF_WP.pdf
>
> We should not assign new memory here. Nor should we copy anything here.
> We should simply use the relocation table to update the indicated memory
> locations.
>
> Best regards
>
> Heinrich
>
>
> > /* Load sections into RAM */
> > for (i = num_sections - 1; i >= 0; i--) {
> > IMAGE_SECTION_HEADER *sec = §ions[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;
> > }
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values
2018-10-12 0:55 ` AKASHI Takahiro
@ 2018-12-02 22:46 ` Alexander Graf
2018-12-03 8:26 ` AKASHI Takahiro
0 siblings, 1 reply; 8+ messages in thread
From: Alexander Graf @ 2018-12-02 22:46 UTC (permalink / raw)
To: u-boot
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.akashi@linaro.org>
>>> Reported-by: Heinrich Schuchardt <xypron.glpk@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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values
2018-12-02 22:46 ` Alexander Graf
@ 2018-12-03 8:26 ` AKASHI Takahiro
2018-12-14 1:18 ` AKASHI Takahiro
0 siblings, 1 reply; 8+ messages in thread
From: AKASHI Takahiro @ 2018-12-03 8:26 UTC (permalink / raw)
To: u-boot
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 <takahiro.akashi@linaro.org>
> >>> Reported-by: Heinrich Schuchardt <xypron.glpk@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?
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values
2018-12-03 8:26 ` AKASHI Takahiro
@ 2018-12-14 1:18 ` AKASHI Takahiro
0 siblings, 0 replies; 8+ messages in thread
From: AKASHI Takahiro @ 2018-12-14 1:18 UTC (permalink / raw)
To: u-boot
Heinrich,
On Mon, Dec 03, 2018 at 05:26:02PM +0900, AKASHI Takahiro wrote:
> 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 <takahiro.akashi@linaro.org>
> > >>> Reported-by: Heinrich Schuchardt <xypron.glpk@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?
>
> 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.
Do you have any comment here?
-Takahiro Akashi
> Thanks,
> -Takahiro Akashi
>
> >
> > Alex
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values
2018-10-11 11:11 [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values AKASHI Takahiro
2018-10-11 14:18 ` Heinrich Schuchardt
@ 2019-03-21 20:24 ` Heinrich Schuchardt
2019-03-22 0:31 ` AKASHI Takahiro
1 sibling, 1 reply; 8+ messages in thread
From: Heinrich Schuchardt @ 2019-03-21 20:24 UTC (permalink / raw)
To: u-boot
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 <takahiro.akashi@linaro.org>
> Reported-by: Heinrich Schuchardt <xypron.glpk@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 is it needed to copy the header? Did you experience any error
related to this header not copied?
Best regards
Heinrich
> /* Load sections into RAM */
> for (i = num_sections - 1; i >= 0; i--) {
> IMAGE_SECTION_HEADER *sec = §ions[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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values
2019-03-21 20:24 ` Heinrich Schuchardt
@ 2019-03-22 0:31 ` AKASHI Takahiro
0 siblings, 0 replies; 8+ messages in thread
From: AKASHI Takahiro @ 2019-03-22 0:31 UTC (permalink / raw)
To: u-boot
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 <takahiro.akashi@linaro.org>
> > Reported-by: Heinrich Schuchardt <xypron.glpk@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 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 = §ions[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;
> > }
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-03-22 0:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-11 11:11 [U-Boot] [PATCH v2] efi_loader: set image_base and image_size to correct values AKASHI Takahiro
2018-10-11 14:18 ` Heinrich Schuchardt
2018-10-12 0:55 ` AKASHI Takahiro
2018-12-02 22:46 ` Alexander Graf
2018-12-03 8:26 ` AKASHI Takahiro
2018-12-14 1:18 ` AKASHI Takahiro
2019-03-21 20:24 ` Heinrich Schuchardt
2019-03-22 0:31 ` AKASHI Takahiro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.