diff mbox series

[1/1] efi_loader: set image_base and image_size to correct values

Message ID 20180930052601.7173-1-xypron.glpk@gmx.de
State New
Headers show
Series [1/1] efi_loader: set image_base and image_size to correct values | expand

Commit Message

Heinrich Schuchardt Sept. 30, 2018, 5:26 a.m. UTC 
From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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 <takahiro.akashi@linaro.org>

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 <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_image_loader.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Heinrich Schuchardt Oct. 6, 2018, 7:51 a.m. UTC | #1 
On 09/30/2018 07:26 AM, Heinrich Schuchardt wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> 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 <takahiro.akashi@linaro.org>
> 
> 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 <xypron.glpk@gmx.de>
> ---
>  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

## 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;
  coff_header = &(header->coff_header);
  sections
    = (struct grub_pe32_section_table *) ((char *) coff_header
                                          + sizeof (*coff_header)
                                          +
coff_header->optional_header_size);

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
Heinrich Schuchardt Oct. 6, 2018, 8:34 a.m. UTC | #2 
On 10/06/2018 09:51 AM, Heinrich Schuchardt wrote:
> On 09/30/2018 07:26 AM, Heinrich Schuchardt wrote:
>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>
>> 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 <takahiro.akashi@linaro.org>
>>
>> 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 <xypron.glpk@gmx.de>
>> ---
>>  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
> 
> ## 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;
>   coff_header = &(header->coff_header);
>   sections
>     = (struct grub_pe32_section_table *) ((char *) coff_header
>                                           + sizeof (*coff_header)
>                                           +
> coff_header->optional_header_size);
> 
> 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
> 

@Alex:

Why did you decide to copy all loaded image sections before relocation?
Couldn't we just apply the relocations in-situ?

When LoadImage() is called for a file path we always assign new memory.
To stay consistent when LoadImage() is called for a memory image we
should copy it. Then in both cases in-situ relocation will not mess up
the source of the loaded image. And in both cases we own the malloc()ed
memory location and can free() it on Unload() or Exit() in case of an
application.

Best regards

Heinrich
AKASHI Takahiro Oct. 9, 2018, 6:55 a.m. UTC | #3 
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 <takahiro.akashi@linaro.org>
> > 
> > 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 <takahiro.akashi@linaro.org>
> > 
> > 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 <xypron.glpk@gmx.de>
> > ---
> >  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


> 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
> 
> 
>
Heinrich Schuchardt Oct. 9, 2018, 5:25 p.m. UTC | #4 
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 <takahiro.akashi@linaro.org>
>>>
>>> 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 <takahiro.akashi@linaro.org>
>>>
>>> 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 <xypron.glpk@gmx.de>
>>> ---
>>>  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

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. This implies that the relative position
of all sections can remain unchanged and we can do the relocation in
place (w/o copying again). This is also what EDK II does.

So when LoadImage() is called for a file load it and relocate in place.
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
>>
>>
>>
>
AKASHI Takahiro Oct. 10, 2018, 1:42 a.m. UTC | #5 
# 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 <takahiro.akashi@linaro.org>
> >>>
> >>> 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 <takahiro.akashi@linaro.org>
> >>>
> >>> 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 <xypron.glpk@gmx.de>
> >>> ---
> >>>  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
> >>
> >>
> >>
> > 
>
diff mbox series

Patch

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;
 }