[PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Ard Biesheuvel]

After the EFI stub has done its business, it jumps into the kernel by
branching to offset #0 of the loaded Image, which is where it expects
to find the header containing a 'branch to stext' instruction.

However, the UEFI spec 2.1.1 states the following regarding PE/COFF
image loading:
"A UEFI image is loaded into memory through the LoadImage() Boot
Service. This service loads an image with a PE32+ format into memory.
This PE32+ loader is required to load all sections of the PE32+ image
into memory."

In other words, it is /not/ required to load parts of the image that are
not covered by a PE/COFF section, so it may not have loaded the header
at the expected offset, as it is not covered by any PE/COFF section.

So instead, jump to 'stext' directly, which is at the base of the
PE/COFF .text section, by supplying a symbol 'stext_offset' to
efi-entry.o which contains the relative offset of stext into the Image.
Also replace other open coded calculations of the same value with a
reference to 'stext_offset'

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
Changes since v2:
- rebased onto 3.17+
- added spec reference to commit message

Changes since v1:
- drop :lo12: relocation against stext_offset in favor of using a literal
  '=stext_offset' which is safer

 arch/arm64/kernel/efi-entry.S |  3 ++-
 arch/arm64/kernel/head.S      | 10 ++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
index 619b1dd7bcde..a0016d3a17da 100644
--- a/arch/arm64/kernel/efi-entry.S
+++ b/arch/arm64/kernel/efi-entry.S
@@ -61,7 +61,8 @@ ENTRY(efi_stub_entry)
 	 */
 	mov	x20, x0		// DTB address
 	ldr	x0, [sp, #16]	// relocated _text address
-	mov	x21, x0
+	ldr	x21, =stext_offset
+	add	x21, x0, x21
 
 	/*
 	 * Flush dcache covering current runtime addresses
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 0a6e4f924df8..8c06c9d269d2 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -132,6 +132,8 @@ efi_head:
 #endif
 
 #ifdef CONFIG_EFI
+	.globl	stext_offset
+	.set	stext_offset, stext - efi_head
 	.align 3
 pe_header:
 	.ascii	"PE"
@@ -155,7 +157,7 @@ optional_header:
 	.long	0				// SizeOfInitializedData
 	.long	0				// SizeOfUninitializedData
 	.long	efi_stub_entry - efi_head	// AddressOfEntryPoint
-	.long	stext - efi_head		// BaseOfCode
+	.long	stext_offset			// BaseOfCode
 
 extra_header_fields:
 	.quad	0				// ImageBase
@@ -172,7 +174,7 @@ extra_header_fields:
 	.long	_end - efi_head			// SizeOfImage
 
 	// Everything before the kernel image is considered part of the header
-	.long	stext - efi_head		// SizeOfHeaders
+	.long	stext_offset			// SizeOfHeaders
 	.long	0				// CheckSum
 	.short	0xa				// Subsystem (EFI application)
 	.short	0				// DllCharacteristics
@@ -217,9 +219,9 @@ section_table:
 	.byte	0
 	.byte	0        		// end of 0 padding of section name
 	.long	_end - stext		// VirtualSize
-	.long	stext - efi_head	// VirtualAddress
+	.long	stext_offset		// VirtualAddress
 	.long	_edata - stext		// SizeOfRawData
-	.long	stext - efi_head	// PointerToRawData
+	.long	stext_offset		// PointerToRawData
 
 	.long	0		// PointerToRelocations (0 for executables)
 	.long	0		// PointerToLineNumbers (0 for executables)
-- 
1.8.3.2

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Mark Rutland]

Hi Ard,

On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
> After the EFI stub has done its business, it jumps into the kernel by
> branching to offset #0 of the loaded Image, which is where it expects
> to find the header containing a 'branch to stext' instruction.
> 
> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
> image loading:
> "A UEFI image is loaded into memory through the LoadImage() Boot
> Service. This service loads an image with a PE32+ format into memory.
> This PE32+ loader is required to load all sections of the PE32+ image
> into memory."
> 
> In other words, it is /not/ required to load parts of the image that are
> not covered by a PE/COFF section, so it may not have loaded the header
> at the expected offset, as it is not covered by any PE/COFF section.

What does this mean for handle_kernel_image? Given we might not have
_text through to _stext mapped, do we not need to take that into
account?

Also, have we seen problems on any systems yet?

Otherwise, this looks like a good fix for hte problem.

Thanks,
Mark.

> So instead, jump to 'stext' directly, which is at the base of the
> PE/COFF .text section, by supplying a symbol 'stext_offset' to
> efi-entry.o which contains the relative offset of stext into the Image.
> Also replace other open coded calculations of the same value with a
> reference to 'stext_offset'
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Changes since v2:
> - rebased onto 3.17+
> - added spec reference to commit message
> 
> Changes since v1:
> - drop :lo12: relocation against stext_offset in favor of using a literal
>   '=stext_offset' which is safer
> 
>  arch/arm64/kernel/efi-entry.S |  3 ++-
>  arch/arm64/kernel/head.S      | 10 ++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
> index 619b1dd7bcde..a0016d3a17da 100644
> --- a/arch/arm64/kernel/efi-entry.S
> +++ b/arch/arm64/kernel/efi-entry.S
> @@ -61,7 +61,8 @@ ENTRY(efi_stub_entry)
>  	 */
>  	mov	x20, x0		// DTB address
>  	ldr	x0, [sp, #16]	// relocated _text address
> -	mov	x21, x0
> +	ldr	x21, =stext_offset
> +	add	x21, x0, x21
>  
>  	/*
>  	 * Flush dcache covering current runtime addresses
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 0a6e4f924df8..8c06c9d269d2 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -132,6 +132,8 @@ efi_head:
>  #endif
>  
>  #ifdef CONFIG_EFI
> +	.globl	stext_offset
> +	.set	stext_offset, stext - efi_head
>  	.align 3
>  pe_header:
>  	.ascii	"PE"
> @@ -155,7 +157,7 @@ optional_header:
>  	.long	0				// SizeOfInitializedData
>  	.long	0				// SizeOfUninitializedData
>  	.long	efi_stub_entry - efi_head	// AddressOfEntryPoint
> -	.long	stext - efi_head		// BaseOfCode
> +	.long	stext_offset			// BaseOfCode
>  
>  extra_header_fields:
>  	.quad	0				// ImageBase
> @@ -172,7 +174,7 @@ extra_header_fields:
>  	.long	_end - efi_head			// SizeOfImage
>  
>  	// Everything before the kernel image is considered part of the header
> -	.long	stext - efi_head		// SizeOfHeaders
> +	.long	stext_offset			// SizeOfHeaders
>  	.long	0				// CheckSum
>  	.short	0xa				// Subsystem (EFI application)
>  	.short	0				// DllCharacteristics
> @@ -217,9 +219,9 @@ section_table:
>  	.byte	0
>  	.byte	0        		// end of 0 padding of section name
>  	.long	_end - stext		// VirtualSize
> -	.long	stext - efi_head	// VirtualAddress
> +	.long	stext_offset		// VirtualAddress
>  	.long	_edata - stext		// SizeOfRawData
> -	.long	stext - efi_head	// PointerToRawData
> +	.long	stext_offset		// PointerToRawData
>  
>  	.long	0		// PointerToRelocations (0 for executables)
>  	.long	0		// PointerToLineNumbers (0 for executables)
> -- 
> 1.8.3.2
> 
> 

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Ard Biesheuvel]

On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Ard,
>
> On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
>> After the EFI stub has done its business, it jumps into the kernel by
>> branching to offset #0 of the loaded Image, which is where it expects
>> to find the header containing a 'branch to stext' instruction.
>>
>> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
>> image loading:
>> "A UEFI image is loaded into memory through the LoadImage() Boot
>> Service. This service loads an image with a PE32+ format into memory.
>> This PE32+ loader is required to load all sections of the PE32+ image
>> into memory."
>>
>> In other words, it is /not/ required to load parts of the image that are
>> not covered by a PE/COFF section, so it may not have loaded the header
>> at the expected offset, as it is not covered by any PE/COFF section.
>
> What does this mean for handle_kernel_image? Given we might not have
> _text through to _stext mapped, do we not need to take that into
> account?
>

Actually, handle_kernel_image() does not interpret the header, it just
copies it along with the rest of the image if it needs to be
relocated, so I don't see an issue there. However, I do remember Mark
Salter mentioning that there is at least one other location that needs
to be fixed up if this concern is valid. Mark?

> Also, have we seen problems on any systems yet?
>

No, I am not aware of any occurrences of this exact issue, this is
just one of the things I spotted while working on this code.
But I think we mostly agree that branching through the header relies
on behavior of the PE/COFF loader that is not covered by the spec.

-- 
Ard.


>> So instead, jump to 'stext' directly, which is at the base of the
>> PE/COFF .text section, by supplying a symbol 'stext_offset' to
>> efi-entry.o which contains the relative offset of stext into the Image.
>> Also replace other open coded calculations of the same value with a
>> reference to 'stext_offset'
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>> Changes since v2:
>> - rebased onto 3.17+
>> - added spec reference to commit message
>>
>> Changes since v1:
>> - drop :lo12: relocation against stext_offset in favor of using a literal
>>   '=stext_offset' which is safer
>>
>>  arch/arm64/kernel/efi-entry.S |  3 ++-
>>  arch/arm64/kernel/head.S      | 10 ++++++----
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
>> index 619b1dd7bcde..a0016d3a17da 100644
>> --- a/arch/arm64/kernel/efi-entry.S
>> +++ b/arch/arm64/kernel/efi-entry.S
>> @@ -61,7 +61,8 @@ ENTRY(efi_stub_entry)
>>        */
>>       mov     x20, x0         // DTB address
>>       ldr     x0, [sp, #16]   // relocated _text address
>> -     mov     x21, x0
>> +     ldr     x21, =stext_offset
>> +     add     x21, x0, x21
>>
>>       /*
>>        * Flush dcache covering current runtime addresses
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 0a6e4f924df8..8c06c9d269d2 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -132,6 +132,8 @@ efi_head:
>>  #endif
>>
>>  #ifdef CONFIG_EFI
>> +     .globl  stext_offset
>> +     .set    stext_offset, stext - efi_head
>>       .align 3
>>  pe_header:
>>       .ascii  "PE"
>> @@ -155,7 +157,7 @@ optional_header:
>>       .long   0                               // SizeOfInitializedData
>>       .long   0                               // SizeOfUninitializedData
>>       .long   efi_stub_entry - efi_head       // AddressOfEntryPoint
>> -     .long   stext - efi_head                // BaseOfCode
>> +     .long   stext_offset                    // BaseOfCode
>>
>>  extra_header_fields:
>>       .quad   0                               // ImageBase
>> @@ -172,7 +174,7 @@ extra_header_fields:
>>       .long   _end - efi_head                 // SizeOfImage
>>
>>       // Everything before the kernel image is considered part of the header
>> -     .long   stext - efi_head                // SizeOfHeaders
>> +     .long   stext_offset                    // SizeOfHeaders
>>       .long   0                               // CheckSum
>>       .short  0xa                             // Subsystem (EFI application)
>>       .short  0                               // DllCharacteristics
>> @@ -217,9 +219,9 @@ section_table:
>>       .byte   0
>>       .byte   0                       // end of 0 padding of section name
>>       .long   _end - stext            // VirtualSize
>> -     .long   stext - efi_head        // VirtualAddress
>> +     .long   stext_offset            // VirtualAddress
>>       .long   _edata - stext          // SizeOfRawData
>> -     .long   stext - efi_head        // PointerToRawData
>> +     .long   stext_offset            // PointerToRawData
>>
>>       .long   0               // PointerToRelocations (0 for executables)
>>       .long   0               // PointerToLineNumbers (0 for executables)
>> --
>> 1.8.3.2
>>
>>

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Mark Salter]

On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote:
> On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > Hi Ard,
> >
> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
> >> After the EFI stub has done its business, it jumps into the kernel by
> >> branching to offset #0 of the loaded Image, which is where it expects
> >> to find the header containing a 'branch to stext' instruction.
> >>
> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
> >> image loading:
> >> "A UEFI image is loaded into memory through the LoadImage() Boot
> >> Service. This service loads an image with a PE32+ format into memory.
> >> This PE32+ loader is required to load all sections of the PE32+ image
> >> into memory."
> >>
> >> In other words, it is /not/ required to load parts of the image that are
> >> not covered by a PE/COFF section, so it may not have loaded the header
> >> at the expected offset, as it is not covered by any PE/COFF section.
> >
> > What does this mean for handle_kernel_image? Given we might not have
> > _text through to _stext mapped, do we not need to take that into
> > account?
> >
> 
> Actually, handle_kernel_image() does not interpret the header, it just
> copies it along with the rest of the image if it needs to be
> relocated, so I don't see an issue there. However, I do remember Mark
> Salter mentioning that there is at least one other location that needs
> to be fixed up if this concern is valid. Mark?

I think at one time we were using the the EFI_LOADED_IMAGE_PROTOCOL
ImageBase field and assuming it pointed to the start of the copied
file, but I don't think we do so currently. My thought at the time was
that the ImageBase pointer didn't really make sense unless the whole
image file was copied by LoadImage(). The description for LoadImage has:

  The LoadImage() function loads an EFI image into memory and returns
  a handle to the image. The image is loaded in one of two ways.

  • If SourceBuffer is not NULL, the function is a memory-to-memory
    load  in which SourceBuffer points to the image to be loaded and
    SourceSize indicates the image’s size in bytes. In this case, the
    caller has copied the image into SourceBuffer and can free the
    buffer once loading is complete.

  • If SourceBuffer is NULL, the function is a file copy operation that
    uses the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.

Which makes me think the whole thing gets copied. But in any case, I
have no objection to your patch.

> 
> > Also, have we seen problems on any systems yet?
> >
> 
> No, I am not aware of any occurrences of this exact issue, this is
> just one of the things I spotted while working on this code.
> But I think we mostly agree that branching through the header relies
> on behavior of the PE/COFF loader that is not covered by the spec.
> 

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Roy Franz]

On Thu, Oct 9, 2014 at 3:19 PM, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote:
>> On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> > Hi Ard,
>> >
>> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
>> >> After the EFI stub has done its business, it jumps into the kernel by
>> >> branching to offset #0 of the loaded Image, which is where it expects
>> >> to find the header containing a 'branch to stext' instruction.
>> >>
>> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
>> >> image loading:
>> >> "A UEFI image is loaded into memory through the LoadImage() Boot
>> >> Service. This service loads an image with a PE32+ format into memory.
>> >> This PE32+ loader is required to load all sections of the PE32+ image
>> >> into memory."
>> >>
>> >> In other words, it is /not/ required to load parts of the image that are
>> >> not covered by a PE/COFF section, so it may not have loaded the header
>> >> at the expected offset, as it is not covered by any PE/COFF section.
>> >
>> > What does this mean for handle_kernel_image? Given we might not have
>> > _text through to _stext mapped, do we not need to take that into
>> > account?
>> >
>>
>> Actually, handle_kernel_image() does not interpret the header, it just
>> copies it along with the rest of the image if it needs to be
>> relocated, so I don't see an issue there. However, I do remember Mark
>> Salter mentioning that there is at least one other location that needs
>> to be fixed up if this concern is valid. Mark?
>
> I think at one time we were using the the EFI_LOADED_IMAGE_PROTOCOL
> ImageBase field and assuming it pointed to the start of the copied
> file, but I don't think we do so currently. My thought at the time was
> that the ImageBase pointer didn't really make sense unless the whole
> image file was copied by LoadImage(). The description for LoadImage has:
>
>   The LoadImage() function loads an EFI image into memory and returns
>   a handle to the image. The image is loaded in one of two ways.
>
>   • If SourceBuffer is not NULL, the function is a memory-to-memory
>     load  in which SourceBuffer points to the image to be loaded and
>     SourceSize indicates the image’s size in bytes. In this case, the
>     caller has copied the image into SourceBuffer and can free the
>     buffer once loading is complete.
>
>   • If SourceBuffer is NULL, the function is a file copy operation that
>     uses the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
>
> Which makes me think the whole thing gets copied. But in any case, I
> have no objection to your patch.
>
In practice for the edk2, the whole thing does get loaded into memory.
Whether this
is mandated by the UEFI specification seems less clear.

I just took a look at handle_kernel_image(), and I don't think it will do
the right thing if the firmware doesn't load the header.  Looking deeper,
I think some other stuff will be broken in that case as well.
handle_kernel_image() takes the image_addr, which the ASM wrapper
computes using PC relative
operations and _text:

adrp x8, _text
add x8, x8, #:lo12:_text

So if the header is actually not loaded, this address will be wrong -
it will be outside
the area loaded. While we're not using the EFI_LOADED_IMAGE_PROTOCOL,
our calculation is only
correct if it does load the whole file.

I think a similar error would be present in handle_kernel_image() with regard
to determining how big the kernel is so it can be copied:
kernel_size = _edata - _text;
This is not correct amount to copy if the header isn't loaded.

I think we'll also run into some alignment issues if the loader really just
loads the section ( we just have the one) rather than the whole file.
From reviewing the PE/COFF spec again we violate the relationship
between FileAlignment and SectionAlignment.

I think the patch is fine for what it does - avoids executing the
branch in the PE/COFF header,
and rather branching directly to the desired code that is in the
PE/COFF section.
There are variety of other issues that would need to be addressed if
the EFI loader
is just loading the bare section.

Roy


>>
>> > Also, have we seen problems on any systems yet?
>> >
>>
>> No, I am not aware of any occurrences of this exact issue, this is
>> just one of the things I spotted while working on this code.
>> But I think we mostly agree that branching through the header relies
>> on behavior of the PE/COFF loader that is not covered by the spec.
>>
>
>

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Ard Biesheuvel]

On 10 October 2014 01:20, Roy Franz <roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Oct 9, 2014 at 3:19 PM, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote:
>>> On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>>> > Hi Ard,
>>> >
>>> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
>>> >> After the EFI stub has done its business, it jumps into the kernel by
>>> >> branching to offset #0 of the loaded Image, which is where it expects
>>> >> to find the header containing a 'branch to stext' instruction.
>>> >>
>>> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
>>> >> image loading:
>>> >> "A UEFI image is loaded into memory through the LoadImage() Boot
>>> >> Service. This service loads an image with a PE32+ format into memory.
>>> >> This PE32+ loader is required to load all sections of the PE32+ image
>>> >> into memory."
>>> >>
>>> >> In other words, it is /not/ required to load parts of the image that are
>>> >> not covered by a PE/COFF section, so it may not have loaded the header
>>> >> at the expected offset, as it is not covered by any PE/COFF section.
>>> >
>>> > What does this mean for handle_kernel_image? Given we might not have
>>> > _text through to _stext mapped, do we not need to take that into
>>> > account?
>>> >
>>>
>>> Actually, handle_kernel_image() does not interpret the header, it just
>>> copies it along with the rest of the image if it needs to be
>>> relocated, so I don't see an issue there. However, I do remember Mark
>>> Salter mentioning that there is at least one other location that needs
>>> to be fixed up if this concern is valid. Mark?
>>
>> I think at one time we were using the the EFI_LOADED_IMAGE_PROTOCOL
>> ImageBase field and assuming it pointed to the start of the copied
>> file, but I don't think we do so currently. My thought at the time was
>> that the ImageBase pointer didn't really make sense unless the whole
>> image file was copied by LoadImage(). The description for LoadImage has:
>>
>>   The LoadImage() function loads an EFI image into memory and returns
>>   a handle to the image. The image is loaded in one of two ways.
>>
>>   • If SourceBuffer is not NULL, the function is a memory-to-memory
>>     load  in which SourceBuffer points to the image to be loaded and
>>     SourceSize indicates the image’s size in bytes. In this case, the
>>     caller has copied the image into SourceBuffer and can free the
>>     buffer once loading is complete.
>>
>>   • If SourceBuffer is NULL, the function is a file copy operation that
>>     uses the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
>>
>> Which makes me think the whole thing gets copied. But in any case, I
>> have no objection to your patch.
>>
> In practice for the edk2, the whole thing does get loaded into memory.
> Whether this
> is mandated by the UEFI specification seems less clear.
>
> I just took a look at handle_kernel_image(), and I don't think it will do
> the right thing if the firmware doesn't load the header.  Looking deeper,
> I think some other stuff will be broken in that case as well.
> handle_kernel_image() takes the image_addr, which the ASM wrapper
> computes using PC relative
> operations and _text:
>
> adrp x8, _text
> add x8, x8, #:lo12:_text
>
> So if the header is actually not loaded, this address will be wrong -
> it will be outside
> the area loaded. While we're not using the EFI_LOADED_IMAGE_PROTOCOL,
> our calculation is only
> correct if it does load the whole file.
>
> I think a similar error would be present in handle_kernel_image() with regard
> to determining how big the kernel is so it can be copied:
> kernel_size = _edata - _text;
> This is not correct amount to copy if the header isn't loaded.
>
> I think we'll also run into some alignment issues if the loader really just
> loads the section ( we just have the one) rather than the whole file.
> From reviewing the PE/COFF spec again we violate the relationship
> between FileAlignment and SectionAlignment.
>
> I think the patch is fine for what it does - avoids executing the
> branch in the PE/COFF header,
> and rather branching directly to the desired code that is in the
> PE/COFF section.
> There are variety of other issues that would need to be addressed if
> the EFI loader
> is just loading the bare section.
>

The issue is *not* that the PE/COFF .text section may get loaded at
ImageBase instead of at ImageBase+sizeof(header), so I don't think
there is reason for any of your concerns.
The issue I try to address here is where the loader allocates some
memory starting at ImageBase, but only populates those regions that
are covered by a section, i.e., only the .text section in our case.
Copying the header from the file to memory at the same relative
(negative) offset from .text as it happens to appear in the file is
behavior that is not covered by the spec at all.

-- 
Ard.


>>>
>>> > Also, have we seen problems on any systems yet?
>>> >
>>>
>>> No, I am not aware of any occurrences of this exact issue, this is
>>> just one of the things I spotted while working on this code.
>>> But I think we mostly agree that branching through the header relies
>>> on behavior of the PE/COFF loader that is not covered by the spec.
>>>
>>
>>

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Mark Salter]

On Fri, 2014-10-10 at 08:30 +0200, Ard Biesheuvel wrote:
> On 10 October 2014 01:20, Roy Franz <roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, Oct 9, 2014 at 3:19 PM, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote:
> >>> On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> >>> > Hi Ard,
> >>> >
> >>> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
> >>> >> After the EFI stub has done its business, it jumps into the kernel by
> >>> >> branching to offset #0 of the loaded Image, which is where it expects
> >>> >> to find the header containing a 'branch to stext' instruction.
> >>> >>
> >>> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
> >>> >> image loading:
> >>> >> "A UEFI image is loaded into memory through the LoadImage() Boot
> >>> >> Service. This service loads an image with a PE32+ format into memory.
> >>> >> This PE32+ loader is required to load all sections of the PE32+ image
> >>> >> into memory."
> >>> >>
> >>> >> In other words, it is /not/ required to load parts of the image that are
> >>> >> not covered by a PE/COFF section, so it may not have loaded the header
> >>> >> at the expected offset, as it is not covered by any PE/COFF section.
> >>> >
> >>> > What does this mean for handle_kernel_image? Given we might not have
> >>> > _text through to _stext mapped, do we not need to take that into
> >>> > account?
> >>> >
> >>>
> >>> Actually, handle_kernel_image() does not interpret the header, it just
> >>> copies it along with the rest of the image if it needs to be
> >>> relocated, so I don't see an issue there. However, I do remember Mark
> >>> Salter mentioning that there is at least one other location that needs
> >>> to be fixed up if this concern is valid. Mark?
> >>
> >> I think at one time we were using the the EFI_LOADED_IMAGE_PROTOCOL
> >> ImageBase field and assuming it pointed to the start of the copied
> >> file, but I don't think we do so currently. My thought at the time was
> >> that the ImageBase pointer didn't really make sense unless the whole
> >> image file was copied by LoadImage(). The description for LoadImage has:
> >>
> >>   The LoadImage() function loads an EFI image into memory and returns
> >>   a handle to the image. The image is loaded in one of two ways.
> >>
> >>   • If SourceBuffer is not NULL, the function is a memory-to-memory
> >>     load  in which SourceBuffer points to the image to be loaded and
> >>     SourceSize indicates the image’s size in bytes. In this case, the
> >>     caller has copied the image into SourceBuffer and can free the
> >>     buffer once loading is complete.
> >>
> >>   • If SourceBuffer is NULL, the function is a file copy operation that
> >>     uses the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> >>
> >> Which makes me think the whole thing gets copied. But in any case, I
> >> have no objection to your patch.
> >>
> > In practice for the edk2, the whole thing does get loaded into memory.
> > Whether this
> > is mandated by the UEFI specification seems less clear.
> >
> > I just took a look at handle_kernel_image(), and I don't think it will do
> > the right thing if the firmware doesn't load the header.  Looking deeper,
> > I think some other stuff will be broken in that case as well.
> > handle_kernel_image() takes the image_addr, which the ASM wrapper
> > computes using PC relative
> > operations and _text:
> >
> > adrp x8, _text
> > add x8, x8, #:lo12:_text
> >
> > So if the header is actually not loaded, this address will be wrong -
> > it will be outside
> > the area loaded. While we're not using the EFI_LOADED_IMAGE_PROTOCOL,
> > our calculation is only
> > correct if it does load the whole file.
> >
> > I think a similar error would be present in handle_kernel_image() with regard
> > to determining how big the kernel is so it can be copied:
> > kernel_size = _edata - _text;
> > This is not correct amount to copy if the header isn't loaded.
> >
> > I think we'll also run into some alignment issues if the loader really just
> > loads the section ( we just have the one) rather than the whole file.
> > From reviewing the PE/COFF spec again we violate the relationship
> > between FileAlignment and SectionAlignment.
> >
> > I think the patch is fine for what it does - avoids executing the
> > branch in the PE/COFF header,
> > and rather branching directly to the desired code that is in the
> > PE/COFF section.
> > There are variety of other issues that would need to be addressed if
> > the EFI loader
> > is just loading the bare section.
> >
> 
> The issue is *not* that the PE/COFF .text section may get loaded at
> ImageBase instead of at ImageBase+sizeof(header), so I don't think
> there is reason for any of your concerns.
> The issue I try to address here is where the loader allocates some
> memory starting at ImageBase, but only populates those regions that
> are covered by a section, i.e., only the .text section in our case.
> Copying the header from the file to memory at the same relative
> (negative) offset from .text as it happens to appear in the file is
> behavior that is not covered by the spec at all.
> 

I think we need a loader that actually does this so all of these
patches can be tested in that scenario. Otherwise, there are likely
going to be other bugs related to it. If we're going to address that
possibility, it would be nice to know that it is actually fixed.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Ard Biesheuvel]

On 10 October 2014 16:14, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 2014-10-10 at 08:30 +0200, Ard Biesheuvel wrote:
>> On 10 October 2014 01:20, Roy Franz <roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> > On Thu, Oct 9, 2014 at 3:19 PM, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >> On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote:
>> >>> On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> >>> > Hi Ard,
>> >>> >
>> >>> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
>> >>> >> After the EFI stub has done its business, it jumps into the kernel by
>> >>> >> branching to offset #0 of the loaded Image, which is where it expects
>> >>> >> to find the header containing a 'branch to stext' instruction.
>> >>> >>
>> >>> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
>> >>> >> image loading:
>> >>> >> "A UEFI image is loaded into memory through the LoadImage() Boot
>> >>> >> Service. This service loads an image with a PE32+ format into memory.
>> >>> >> This PE32+ loader is required to load all sections of the PE32+ image
>> >>> >> into memory."
>> >>> >>
>> >>> >> In other words, it is /not/ required to load parts of the image that are
>> >>> >> not covered by a PE/COFF section, so it may not have loaded the header
>> >>> >> at the expected offset, as it is not covered by any PE/COFF section.
>> >>> >
>> >>> > What does this mean for handle_kernel_image? Given we might not have
>> >>> > _text through to _stext mapped, do we not need to take that into
>> >>> > account?
>> >>> >
>> >>>
>> >>> Actually, handle_kernel_image() does not interpret the header, it just
>> >>> copies it along with the rest of the image if it needs to be
>> >>> relocated, so I don't see an issue there. However, I do remember Mark
>> >>> Salter mentioning that there is at least one other location that needs
>> >>> to be fixed up if this concern is valid. Mark?
>> >>
>> >> I think at one time we were using the the EFI_LOADED_IMAGE_PROTOCOL
>> >> ImageBase field and assuming it pointed to the start of the copied
>> >> file, but I don't think we do so currently. My thought at the time was
>> >> that the ImageBase pointer didn't really make sense unless the whole
>> >> image file was copied by LoadImage(). The description for LoadImage has:
>> >>
>> >>   The LoadImage() function loads an EFI image into memory and returns
>> >>   a handle to the image. The image is loaded in one of two ways.
>> >>
>> >>   • If SourceBuffer is not NULL, the function is a memory-to-memory
>> >>     load  in which SourceBuffer points to the image to be loaded and
>> >>     SourceSize indicates the image’s size in bytes. In this case, the
>> >>     caller has copied the image into SourceBuffer and can free the
>> >>     buffer once loading is complete.
>> >>
>> >>   • If SourceBuffer is NULL, the function is a file copy operation that
>> >>     uses the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
>> >>
>> >> Which makes me think the whole thing gets copied. But in any case, I
>> >> have no objection to your patch.
>> >>
>> > In practice for the edk2, the whole thing does get loaded into memory.
>> > Whether this
>> > is mandated by the UEFI specification seems less clear.
>> >
>> > I just took a look at handle_kernel_image(), and I don't think it will do
>> > the right thing if the firmware doesn't load the header.  Looking deeper,
>> > I think some other stuff will be broken in that case as well.
>> > handle_kernel_image() takes the image_addr, which the ASM wrapper
>> > computes using PC relative
>> > operations and _text:
>> >
>> > adrp x8, _text
>> > add x8, x8, #:lo12:_text
>> >
>> > So if the header is actually not loaded, this address will be wrong -
>> > it will be outside
>> > the area loaded. While we're not using the EFI_LOADED_IMAGE_PROTOCOL,
>> > our calculation is only
>> > correct if it does load the whole file.
>> >
>> > I think a similar error would be present in handle_kernel_image() with regard
>> > to determining how big the kernel is so it can be copied:
>> > kernel_size = _edata - _text;
>> > This is not correct amount to copy if the header isn't loaded.
>> >
>> > I think we'll also run into some alignment issues if the loader really just
>> > loads the section ( we just have the one) rather than the whole file.
>> > From reviewing the PE/COFF spec again we violate the relationship
>> > between FileAlignment and SectionAlignment.
>> >
>> > I think the patch is fine for what it does - avoids executing the
>> > branch in the PE/COFF header,
>> > and rather branching directly to the desired code that is in the
>> > PE/COFF section.
>> > There are variety of other issues that would need to be addressed if
>> > the EFI loader
>> > is just loading the bare section.
>> >
>>
>> The issue is *not* that the PE/COFF .text section may get loaded at
>> ImageBase instead of at ImageBase+sizeof(header), so I don't think
>> there is reason for any of your concerns.
>> The issue I try to address here is where the loader allocates some
>> memory starting at ImageBase, but only populates those regions that
>> are covered by a section, i.e., only the .text section in our case.
>> Copying the header from the file to memory at the same relative
>> (negative) offset from .text as it happens to appear in the file is
>> behavior that is not covered by the spec at all.
>>
>
> I think we need a loader that actually does this so all of these
> patches can be tested in that scenario. Otherwise, there are likely
> going to be other bugs related to it. If we're going to address that
> possibility, it would be nice to know that it is actually fixed.
>

What I found is that you can easily test this by offsetting the
virtual address of the .text section by e.g., 0x1000. This results in
a hole in the memory image between the header and the .text section
that does not exist in the file, so negative offsets from .text (which
is essentially how we access the header currently) point into the
hole, not at the header.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Peter Jones]

On Thu, Oct 09, 2014 at 04:20:19PM -0700, Roy Franz wrote:
> I think we'll also run into some alignment issues if the loader really just
> loads the section ( we just have the one) rather than the whole file.
> From reviewing the PE/COFF spec again we violate the relationship
> between FileAlignment and SectionAlignment.

It's hard to pin down exactly whose burden this is - clearly the RVAs
and the File Addresses in the binary are supposed to jive with
SectionAlignment and FileAlignment, respectively, but if they don't,
it's unclear what a loader should do about it.  I've got some (x86)
hardware where LoadImage() rejects the binary entirely in this case, but
the code in tiano doesn't seem to care.  Our loader code in shim ignores
both fields entirely - once we've checked validity in terms of overlap
and boundaries, we happily give sections whatever offset they've
requested.  I suppose on some machines that may mean that a malformed
binary gets a /fault/ pretty quickly.  I'll add checks for those
alignments in shim 0.9 , release date TBD.

So because of that x86 machine that does error out, I've got some
different values here:
https://github.com/vathpela/shim/blob/manual-headers/crt0-efi-x86_64.S#L72
than Ard does in his Aarch64 headers.  There are some other things
wrong on that branch because I didn't understand well enough while
writing some of it - it's basically still there for reference, I don't
anticipate ever shipping it.

-- 
        Peter

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Mark Rutland]

On Thu, Oct 09, 2014 at 08:03:52PM +0100, Ard Biesheuvel wrote:
> On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > Hi Ard,
> >
> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
> >> After the EFI stub has done its business, it jumps into the kernel by
> >> branching to offset #0 of the loaded Image, which is where it expects
> >> to find the header containing a 'branch to stext' instruction.
> >>
> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
> >> image loading:
> >> "A UEFI image is loaded into memory through the LoadImage() Boot
> >> Service. This service loads an image with a PE32+ format into memory.
> >> This PE32+ loader is required to load all sections of the PE32+ image
> >> into memory."
> >>
> >> In other words, it is /not/ required to load parts of the image that are
> >> not covered by a PE/COFF section, so it may not have loaded the header
> >> at the expected offset, as it is not covered by any PE/COFF section.
> >
> > What does this mean for handle_kernel_image? Given we might not have
> > _text through to _stext mapped, do we not need to take that into
> > account?
> >
> 
> Actually, handle_kernel_image() does not interpret the header, it just
> copies it along with the rest of the image if it needs to be
> relocated, so I don't see an issue there.

Sorry, I wasn't clear enough with my concern. My concern was whether we
had any guarantee _something_ was mapped for the address range covering
efi_head to stext.

So long as _something_ is mapped there, we're ok -- handle_kernel_image
will just copy some garbage along with the usable portion of the kernel.

But if the EFI loader is allowed to load stext at the precise start of
RAM (or anywhere not in the idmap), in attempting the copy we'd try to
access unmapped addresses.

So if that's a possibility, we need to shrink the copy to cover stext
to _edata rather than _text to edata.

Does that make sense?

> However, I do remember Mark
> Salter mentioning that there is at least one other location that needs
> to be fixed up if this concern is valid. Mark?
> 
> > Also, have we seen problems on any systems yet?
> >
> 
> No, I am not aware of any occurrences of this exact issue, this is
> just one of the things I spotted while working on this code.

Ok. I was just curious as to how urgent this was.

> But I think we mostly agree that branching through the header relies
> on behavior of the PE/COFF loader that is not covered by the spec.

Yes. We should not rely on unspecified behaviour.

Mark.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Ard Biesheuvel]

On 10 October 2014 12:49, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Thu, Oct 09, 2014 at 08:03:52PM +0100, Ard Biesheuvel wrote:
>> On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> > Hi Ard,
>> >
>> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
>> >> After the EFI stub has done its business, it jumps into the kernel by
>> >> branching to offset #0 of the loaded Image, which is where it expects
>> >> to find the header containing a 'branch to stext' instruction.
>> >>
>> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
>> >> image loading:
>> >> "A UEFI image is loaded into memory through the LoadImage() Boot
>> >> Service. This service loads an image with a PE32+ format into memory.
>> >> This PE32+ loader is required to load all sections of the PE32+ image
>> >> into memory."
>> >>
>> >> In other words, it is /not/ required to load parts of the image that are
>> >> not covered by a PE/COFF section, so it may not have loaded the header
>> >> at the expected offset, as it is not covered by any PE/COFF section.
>> >
>> > What does this mean for handle_kernel_image? Given we might not have
>> > _text through to _stext mapped, do we not need to take that into
>> > account?
>> >
>>
>> Actually, handle_kernel_image() does not interpret the header, it just
>> copies it along with the rest of the image if it needs to be
>> relocated, so I don't see an issue there.
>
> Sorry, I wasn't clear enough with my concern. My concern was whether we
> had any guarantee _something_ was mapped for the address range covering
> efi_head to stext.
>
> So long as _something_ is mapped there, we're ok -- handle_kernel_image
> will just copy some garbage along with the usable portion of the kernel.
>

Indeed.

> But if the EFI loader is allowed to load stext at the precise start of
> RAM (or anywhere not in the idmap), in attempting the copy we'd try to
> access unmapped addresses.
>
> So if that's a possibility, we need to shrink the copy to cover stext
> to _edata rather than _text to edata.
>
> Does that make sense?
>

That cannot happen. The PE/COFF .text section's positive relative
virtual offset ensures that the memory image has room for the header,
it's just not guaranteed that anything gets copied there.

>> However, I do remember Mark
>> Salter mentioning that there is at least one other location that needs
>> to be fixed up if this concern is valid. Mark?
>>
>> > Also, have we seen problems on any systems yet?
>> >
>>
>> No, I am not aware of any occurrences of this exact issue, this is
>> just one of the things I spotted while working on this code.
>
> Ok. I was just curious as to how urgent this was.
>
>> But I think we mostly agree that branching through the header relies
>> on behavior of the PE/COFF loader that is not covered by the spec.
>
> Yes. We should not rely on unspecified behaviour.
>
> Mark.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Mark Rutland]

On Fri, Oct 10, 2014 at 12:52:32PM +0100, Ard Biesheuvel wrote:
> On 10 October 2014 12:49, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Thu, Oct 09, 2014 at 08:03:52PM +0100, Ard Biesheuvel wrote:
> >> On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> >> > Hi Ard,
> >> >
> >> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
> >> >> After the EFI stub has done its business, it jumps into the kernel by
> >> >> branching to offset #0 of the loaded Image, which is where it expects
> >> >> to find the header containing a 'branch to stext' instruction.
> >> >>
> >> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
> >> >> image loading:
> >> >> "A UEFI image is loaded into memory through the LoadImage() Boot
> >> >> Service. This service loads an image with a PE32+ format into memory.
> >> >> This PE32+ loader is required to load all sections of the PE32+ image
> >> >> into memory."
> >> >>
> >> >> In other words, it is /not/ required to load parts of the image that are
> >> >> not covered by a PE/COFF section, so it may not have loaded the header
> >> >> at the expected offset, as it is not covered by any PE/COFF section.
> >> >
> >> > What does this mean for handle_kernel_image? Given we might not have
> >> > _text through to _stext mapped, do we not need to take that into
> >> > account?
> >> >
> >>
> >> Actually, handle_kernel_image() does not interpret the header, it just
> >> copies it along with the rest of the image if it needs to be
> >> relocated, so I don't see an issue there.
> >
> > Sorry, I wasn't clear enough with my concern. My concern was whether we
> > had any guarantee _something_ was mapped for the address range covering
> > efi_head to stext.
> >
> > So long as _something_ is mapped there, we're ok -- handle_kernel_image
> > will just copy some garbage along with the usable portion of the kernel.
> >
> 
> Indeed.
> 
> > But if the EFI loader is allowed to load stext at the precise start of
> > RAM (or anywhere not in the idmap), in attempting the copy we'd try to
> > access unmapped addresses.
> >
> > So if that's a possibility, we need to shrink the copy to cover stext
> > to _edata rather than _text to edata.
> >
> > Does that make sense?
> >
> 
> That cannot happen. The PE/COFF .text section's positive relative
> virtual offset ensures that the memory image has room for the header,
> it's just not guaranteed that anything gets copied there.

Ok. If we're guaranteed to have some space there, we're fine.

I'm probably being a bit thick here, but where is the "positive relative
virtual offset" in the header? Which field defines that?

Thanks,
Mark.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Ard Biesheuvel]

On 10 October 2014 14:19, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Fri, Oct 10, 2014 at 12:52:32PM +0100, Ard Biesheuvel wrote:
>> On 10 October 2014 12:49, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> > On Thu, Oct 09, 2014 at 08:03:52PM +0100, Ard Biesheuvel wrote:
>> >> On 9 October 2014 19:23, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> >> > Hi Ard,
>> >> >
>> >> > On Wed, Oct 08, 2014 at 03:11:27PM +0100, Ard Biesheuvel wrote:
>> >> >> After the EFI stub has done its business, it jumps into the kernel by
>> >> >> branching to offset #0 of the loaded Image, which is where it expects
>> >> >> to find the header containing a 'branch to stext' instruction.
>> >> >>
>> >> >> However, the UEFI spec 2.1.1 states the following regarding PE/COFF
>> >> >> image loading:
>> >> >> "A UEFI image is loaded into memory through the LoadImage() Boot
>> >> >> Service. This service loads an image with a PE32+ format into memory.
>> >> >> This PE32+ loader is required to load all sections of the PE32+ image
>> >> >> into memory."
>> >> >>
>> >> >> In other words, it is /not/ required to load parts of the image that are
>> >> >> not covered by a PE/COFF section, so it may not have loaded the header
>> >> >> at the expected offset, as it is not covered by any PE/COFF section.
>> >> >
>> >> > What does this mean for handle_kernel_image? Given we might not have
>> >> > _text through to _stext mapped, do we not need to take that into
>> >> > account?
>> >> >
>> >>
>> >> Actually, handle_kernel_image() does not interpret the header, it just
>> >> copies it along with the rest of the image if it needs to be
>> >> relocated, so I don't see an issue there.
>> >
>> > Sorry, I wasn't clear enough with my concern. My concern was whether we
>> > had any guarantee _something_ was mapped for the address range covering
>> > efi_head to stext.
>> >
>> > So long as _something_ is mapped there, we're ok -- handle_kernel_image
>> > will just copy some garbage along with the usable portion of the kernel.
>> >
>>
>> Indeed.
>>
>> > But if the EFI loader is allowed to load stext at the precise start of
>> > RAM (or anywhere not in the idmap), in attempting the copy we'd try to
>> > access unmapped addresses.
>> >
>> > So if that's a possibility, we need to shrink the copy to cover stext
>> > to _edata rather than _text to edata.
>> >
>> > Does that make sense?
>> >
>>
>> That cannot happen. The PE/COFF .text section's positive relative
>> virtual offset ensures that the memory image has room for the header,
>> it's just not guaranteed that anything gets copied there.
>
> Ok. If we're guaranteed to have some space there, we're fine.
>
> I'm probably being a bit thick here, but where is the "positive relative
> virtual offset" in the header? Which field defines that?
>

The fields VirtualSize, VirtualAddress (the field I was referring to),
SizeOfRawData and PointerToRawData define the relation between the
file layout and the memory layout of the .text section (line 219 and
up in head.S)

In our current definition, the memory offset and the file offset are
identical (which this patch redefines as 'stext_offset'). The virtual
size covers the entire static memory footprint of Image (minus the
header). whereas the SizeOfRawData contains the size of the payload in
the file (again, minus the header). The balance is zero initialized by
the loader.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Mark Rutland]

[...]

> >> > But if the EFI loader is allowed to load stext at the precise start of
> >> > RAM (or anywhere not in the idmap), in attempting the copy we'd try to
> >> > access unmapped addresses.
> >> >
> >> > So if that's a possibility, we need to shrink the copy to cover stext
> >> > to _edata rather than _text to edata.
> >> >
> >> > Does that make sense?
> >> >
> >>
> >> That cannot happen. The PE/COFF .text section's positive relative
> >> virtual offset ensures that the memory image has room for the header,
> >> it's just not guaranteed that anything gets copied there.
> >
> > Ok. If we're guaranteed to have some space there, we're fine.
> >
> > I'm probably being a bit thick here, but where is the "positive relative
> > virtual offset" in the header? Which field defines that?
> >
> 
> The fields VirtualSize, VirtualAddress (the field I was referring to),
> SizeOfRawData and PointerToRawData define the relation between the
> file layout and the memory layout of the .text section (line 219 and
> up in head.S)

I guess my confusion is over the semantics of the VirtualAddress field.
If it's treated as an offset, what is that offset relative to in memory?
And what defines that the space covered by that offset is accessible?

> In our current definition, the memory offset and the file offset are
> identical (which this patch redefines as 'stext_offset'). The virtual
> size covers the entire static memory footprint of Image (minus the
> header). whereas the SizeOfRawData contains the size of the payload in
> the file (again, minus the header). The balance is zero initialized by
> the loader.

I can see why this guarantees there is space for stext to _end, but I
don't understand how this guarantees there is a valid mapping for the
region that would otherwise be _head to stext.

Thanks,
Mark.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Ard Biesheuvel]

On 10 October 2014 15:03, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> [...]
>
>> >> > But if the EFI loader is allowed to load stext at the precise start of
>> >> > RAM (or anywhere not in the idmap), in attempting the copy we'd try to
>> >> > access unmapped addresses.
>> >> >
>> >> > So if that's a possibility, we need to shrink the copy to cover stext
>> >> > to _edata rather than _text to edata.
>> >> >
>> >> > Does that make sense?
>> >> >
>> >>
>> >> That cannot happen. The PE/COFF .text section's positive relative
>> >> virtual offset ensures that the memory image has room for the header,
>> >> it's just not guaranteed that anything gets copied there.
>> >
>> > Ok. If we're guaranteed to have some space there, we're fine.
>> >
>> > I'm probably being a bit thick here, but where is the "positive relative
>> > virtual offset" in the header? Which field defines that?
>> >
>>
>> The fields VirtualSize, VirtualAddress (the field I was referring to),
>> SizeOfRawData and PointerToRawData define the relation between the
>> file layout and the memory layout of the .text section (line 219 and
>> up in head.S)
>
> I guess my confusion is over the semantics of the VirtualAddress field.
> If it's treated as an offset, what is that offset relative to in memory?
> And what defines that the space covered by that offset is accessible?
>

The PE/COFF spec 8.3 describes VirtualAddress as

"""
For executable images, the address of the first byte of the section
relative to the image base when the section is loaded into memory.
"""

ImageBase is a field itself in the PE/COFF header, described as

"""
The preferred address of the first byte of image when loaded into
memory; must be a multiple of 64 K.
"""

The SizeOfImage field is described as

"""
The size (in bytes) of the image, including all headers, as the image
is loaded in memory.
"""

My interpretation is that memory needs to be allocated for the header
as well as all sections that have a VirtualSize (including sections
like BSS which don't have a payload in the file)

>> In our current definition, the memory offset and the file offset are
>> identical (which this patch redefines as 'stext_offset'). The virtual
>> size covers the entire static memory footprint of Image (minus the
>> header). whereas the SizeOfRawData contains the size of the payload in
>> the file (again, minus the header). The balance is zero initialized by
>> the loader.
>
> I can see why this guarantees there is space for stext to _end, but I
> don't understand how this guarantees there is a valid mapping for the
> region that would otherwise be _head to stext.
>

The allocation itself is defined in terms of ImageBase/SizeOfImage
(although ImageBase is only a preferred offset)
How this allocation is populated with data (and where the holes are)
is described by the sections.

-- 
Ard.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Mark Rutland]

On Fri, Oct 10, 2014 at 02:27:46PM +0100, Ard Biesheuvel wrote:
> On 10 October 2014 15:03, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > [...]
> >
> >> >> > But if the EFI loader is allowed to load stext at the precise start of
> >> >> > RAM (or anywhere not in the idmap), in attempting the copy we'd try to
> >> >> > access unmapped addresses.
> >> >> >
> >> >> > So if that's a possibility, we need to shrink the copy to cover stext
> >> >> > to _edata rather than _text to edata.
> >> >> >
> >> >> > Does that make sense?
> >> >> >
> >> >>
> >> >> That cannot happen. The PE/COFF .text section's positive relative
> >> >> virtual offset ensures that the memory image has room for the header,
> >> >> it's just not guaranteed that anything gets copied there.
> >> >
> >> > Ok. If we're guaranteed to have some space there, we're fine.
> >> >
> >> > I'm probably being a bit thick here, but where is the "positive relative
> >> > virtual offset" in the header? Which field defines that?
> >> >
> >>
> >> The fields VirtualSize, VirtualAddress (the field I was referring to),
> >> SizeOfRawData and PointerToRawData define the relation between the
> >> file layout and the memory layout of the .text section (line 219 and
> >> up in head.S)
> >
> > I guess my confusion is over the semantics of the VirtualAddress field.
> > If it's treated as an offset, what is that offset relative to in memory?
> > And what defines that the space covered by that offset is accessible?
> >
> 
> The PE/COFF spec 8.3 describes VirtualAddress as
> 
> """
> For executable images, the address of the first byte of the section
> relative to the image base when the section is loaded into memory.
> """
> 
> ImageBase is a field itself in the PE/COFF header, described as
> 
> """
> The preferred address of the first byte of image when loaded into
> memory; must be a multiple of 64 K.
> """

Thanks for the info, this now makes a lot more sense to me.

So that means the .text section is not the start of the image, but is
offset by (stext - efi_head) bytes from the start, covering the header
(regardless of whether it is actually present in the loaded image).

> The SizeOfImage field is described as
> 
> """
> The size (in bytes) of the image, including all headers, as the image
> is loaded in memory.
> """
> 
> My interpretation is that memory needs to be allocated for the header
> as well as all sections that have a VirtualSize (including sections
> like BSS which don't have a payload in the file)

That would match what I understand from reading the above, though it
strikes me as odd that space needs to be allocated for the headers if
they aren't guaranteed to be copied -- it's not defined where they would
live in the image.

> >> In our current definition, the memory offset and the file offset are
> >> identical (which this patch redefines as 'stext_offset'). The virtual
> >> size covers the entire static memory footprint of Image (minus the
> >> header). whereas the SizeOfRawData contains the size of the payload in
> >> the file (again, minus the header). The balance is zero initialized by
> >> the loader.
> >
> > I can see why this guarantees there is space for stext to _end, but I
> > don't understand how this guarantees there is a valid mapping for the
> > region that would otherwise be _head to stext.
> >
> 
> The allocation itself is defined in terms of ImageBase/SizeOfImage
> (although ImageBase is only a preferred offset)
> How this allocation is populated with data (and where the holes are)
> is described by the sections.

Ok. Thanks for putting this information together (it's remarkably
useful), and thanks for putting up with my ignorance of PE/COFF.

Cheers,
Mark.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Roy Franz]

On Fri, Oct 10, 2014 at 6:27 AM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 10 October 2014 15:03, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> [...]
>>
>>> >> > But if the EFI loader is allowed to load stext at the precise start of
>>> >> > RAM (or anywhere not in the idmap), in attempting the copy we'd try to
>>> >> > access unmapped addresses.
>>> >> >
>>> >> > So if that's a possibility, we need to shrink the copy to cover stext
>>> >> > to _edata rather than _text to edata.
>>> >> >
>>> >> > Does that make sense?
>>> >> >
>>> >>
>>> >> That cannot happen. The PE/COFF .text section's positive relative
>>> >> virtual offset ensures that the memory image has room for the header,
>>> >> it's just not guaranteed that anything gets copied there.
>>> >
>>> > Ok. If we're guaranteed to have some space there, we're fine.
>>> >
>>> > I'm probably being a bit thick here, but where is the "positive relative
>>> > virtual offset" in the header? Which field defines that?
>>> >
>>>
>>> The fields VirtualSize, VirtualAddress (the field I was referring to),
>>> SizeOfRawData and PointerToRawData define the relation between the
>>> file layout and the memory layout of the .text section (line 219 and
>>> up in head.S)
>>
>> I guess my confusion is over the semantics of the VirtualAddress field.
>> If it's treated as an offset, what is that offset relative to in memory?
>> And what defines that the space covered by that offset is accessible?
>>
>
> The PE/COFF spec 8.3 describes VirtualAddress as
>
> """
> For executable images, the address of the first byte of the section
> relative to the image base when the section is loaded into memory.
> """
>
> ImageBase is a field itself in the PE/COFF header, described as
>
> """
> The preferred address of the first byte of image when loaded into
> memory; must be a multiple of 64 K.
> """

This seems to strongly suggest that the header should be loaded into memory.
"The first byte of the image" is the header we are talking about.

>
> The SizeOfImage field is described as
>
> """
> The size (in bytes) of the image, including all headers, as the image
> is loaded in memory.
> """
>
> My interpretation is that memory needs to be allocated for the header
> as well as all sections that have a VirtualSize (including sections
> like BSS which don't have a payload in the file)

It does seem odd to require allocation for the header, but not copying the
data.

That said, your patch will handle the case of space being allocated
for the header,
but the header not copied, and it doesn't complicate anything, so I'm
fine with it.
Maybe we should add a comment that we still require memory to be allocated for
where they header would be.


>
>>> In our current definition, the memory offset and the file offset are
>>> identical (which this patch redefines as 'stext_offset'). The virtual
>>> size covers the entire static memory footprint of Image (minus the
>>> header). whereas the SizeOfRawData contains the size of the payload in
>>> the file (again, minus the header). The balance is zero initialized by
>>> the loader.
>>
>> I can see why this guarantees there is space for stext to _end, but I
>> don't understand how this guarantees there is a valid mapping for the
>> region that would otherwise be _head to stext.
>>
>
> The allocation itself is defined in terms of ImageBase/SizeOfImage
> (although ImageBase is only a preferred offset)
> How this allocation is populated with data (and where the holes are)
> is described by the sections.
>
> --
> Ard.

Re: [PATCH v3] arm64/efi: efistub: jump to 'stext' directly, not through the header

[Ard Biesheuvel]

On 10 October 2014 17:38, Roy Franz <roy.franz@linaro.org> wrote:
> On Fri, Oct 10, 2014 at 6:27 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 10 October 2014 15:03, Mark Rutland <mark.rutland@arm.com> wrote:
>>> [...]
>>>
>>>> >> > But if the EFI loader is allowed to load stext at the precise start of
>>>> >> > RAM (or anywhere not in the idmap), in attempting the copy we'd try to
>>>> >> > access unmapped addresses.
>>>> >> >
>>>> >> > So if that's a possibility, we need to shrink the copy to cover stext
>>>> >> > to _edata rather than _text to edata.
>>>> >> >
>>>> >> > Does that make sense?
>>>> >> >
>>>> >>
>>>> >> That cannot happen. The PE/COFF .text section's positive relative
>>>> >> virtual offset ensures that the memory image has room for the header,
>>>> >> it's just not guaranteed that anything gets copied there.
>>>> >
>>>> > Ok. If we're guaranteed to have some space there, we're fine.
>>>> >
>>>> > I'm probably being a bit thick here, but where is the "positive relative
>>>> > virtual offset" in the header? Which field defines that?
>>>> >
>>>>
>>>> The fields VirtualSize, VirtualAddress (the field I was referring to),
>>>> SizeOfRawData and PointerToRawData define the relation between the
>>>> file layout and the memory layout of the .text section (line 219 and
>>>> up in head.S)
>>>
>>> I guess my confusion is over the semantics of the VirtualAddress field.
>>> If it's treated as an offset, what is that offset relative to in memory?
>>> And what defines that the space covered by that offset is accessible?
>>>
>>
>> The PE/COFF spec 8.3 describes VirtualAddress as
>>
>> """
>> For executable images, the address of the first byte of the section
>> relative to the image base when the section is loaded into memory.
>> """
>>
>> ImageBase is a field itself in the PE/COFF header, described as
>>
>> """
>> The preferred address of the first byte of image when loaded into
>> memory; must be a multiple of 64 K.
>> """
>
> This seems to strongly suggest that the header should be loaded into memory.
> "The first byte of the image" is the header we are talking about.
>

Perhaps it does, but look at it this way:
the header is a recipe for how to turn a chunk of memory into an
executable image, i.e it has headers for relocation sections,
text/data sections and bss sections, all of which are used in a
different way to populate or manipulate the memory image. The fact
that our file looks *exactly* like this memory image (or almost
exactly, now that we added some zero padding to the .text section) is
purely coincidental. There is a reason a section header has both a
'PointerToRawData' and a 'VirtualAddress': it is because they are
allowed to be *different*, i.e., a file image and a memory image are
not necessarily identical, and things that have /no/ virtual address
(such as the header) cannot be expected to be present in the memory
image at all, let alone at precisely the same negative offset from
some arbitrary section as it appears in the file.


>>
>> The SizeOfImage field is described as
>>
>> """
>> The size (in bytes) of the image, including all headers, as the image
>> is loaded in memory.
>> """
>>
>> My interpretation is that memory needs to be allocated for the header
>> as well as all sections that have a VirtualSize (including sections
>> like BSS which don't have a payload in the file)
>
> It does seem odd to require allocation for the header, but not copying the
> data.
>
> That said, your patch will handle the case of space being allocated
> for the header,
> but the header not copied, and it doesn't complicate anything, so I'm
> fine with it.
> Maybe we should add a comment that we still require memory to be allocated for
> where they header would be.
>
>
>>
>>>> In our current definition, the memory offset and the file offset are
>>>> identical (which this patch redefines as 'stext_offset'). The virtual
>>>> size covers the entire static memory footprint of Image (minus the
>>>> header). whereas the SizeOfRawData contains the size of the payload in
>>>> the file (again, minus the header). The balance is zero initialized by
>>>> the loader.
>>>
>>> I can see why this guarantees there is space for stext to _end, but I
>>> don't understand how this guarantees there is a valid mapping for the
>>> region that would otherwise be _head to stext.
>>>
>>
>> The allocation itself is defined in terms of ImageBase/SizeOfImage
>> (although ImageBase is only a preferred offset)
>> How this allocation is populated with data (and where the holes are)
>> is described by the sections.
>>
>> --
>> Ard.