arm64/efi: efistub: jump to 'stext' directly, not through the header

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 <[email protected]>

---
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(-)

arch/arm64/kernel/efi-entry.S +2 -1

index 619b1dd7bcde..a0016d3a17da 100644
@@ -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

arch/arm64/kernel/head.S +6 -4

index 0a6e4f924df8..8c06c9d269d2 100644
@@ -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)

Signature · 1 line

1.8.3.2

Hi Ard,

Quoted 11 lines · 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."

Quoted 3 lines

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?

Quoted 81 lines

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 <[email protected]>

---
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(-)

arch/arm64/kernel/efi-entry.S +2 -1

index 619b1dd7bcde..a0016d3a17da 100644
@@ -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

arch/arm64/kernel/head.S +6 -5

index 0a6e4f924df8..8c06c9d269d2 100644
@@ -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

Quoted 14 lines · Mark Rutland

Hi Ard,

Quoted 11 lines · 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."

Quoted 8 lines

Quoted 3 lines

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?

Quoted 3 lines

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.

Quoted 24 lines · Ard Biesheuvel

Quoted 14 lines · Mark Rutland

Hi Ard,

Quoted 11 lines · 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."

Quoted 8 lines

Quoted 3 lines

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?

Quoted 5 lines

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.

From reviewing the PE/COFF spec again we violate the relationship

Quoted 9 lines

Quoted 3 lines

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.

Quoted 46 lines · Mark Salter

Quoted 24 lines · Ard Biesheuvel

Quoted 14 lines · Mark Rutland

Hi Ard,

Quoted 11 lines · 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."

Quoted 8 lines

Quoted 3 lines

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?

Quoted 5 lines

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.

Quoted 6 lines

? 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.

Quoted 11 lines

Quoted 9 lines

Quoted 3 lines

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.

Quoted 83 lines · Roy Franz

Quoted 46 lines · Mark Salter

Quoted 24 lines · Ard Biesheuvel

Quoted 14 lines · Mark Rutland

Hi Ard,

Quoted 11 lines · 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."

Quoted 8 lines

Quoted 3 lines

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?

Quoted 5 lines

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.

Quoted 6 lines

? 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.

Quoted 8 lines

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.

Quoted 93 lines · Ard Biesheuvel

Quoted 83 lines · Roy Franz

Quoted 46 lines · Mark Salter

Quoted 24 lines · Ard Biesheuvel

Quoted 14 lines · Mark Rutland

Hi Ard,

Quoted 11 lines · 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."

Quoted 8 lines

Quoted 3 lines

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?

Quoted 5 lines

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.

Quoted 6 lines

? 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.

Quoted 8 lines

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.

Quoted 10 lines

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.

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.

Quoted 18 lines · Mark Rutland

Hi Ard,

Quoted 14 lines · 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.

Quoted 4 lines

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?

Quoted 2 lines

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.

Quoted 19 lines · Ard Biesheuvel

Quoted 18 lines · Mark Rutland

Hi Ard,

Quoted 14 lines · 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.

Quoted 8 lines

Quoted 4 lines

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?

Quoted 9 lines

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?

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 29 lines · Mark Rutland

Quoted 19 lines · Ard Biesheuvel

Quoted 18 lines · Mark Rutland

Hi Ard,

Quoted 14 lines · 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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 50 lines · Ard Biesheuvel

Quoted 29 lines · Mark Rutland

Quoted 19 lines · Ard Biesheuvel

Quoted 18 lines · Mark Rutland

Hi Ard,

Quoted 14 lines · 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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 9 lines

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?

Quoted 3 lines

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?

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

[...]

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 4 lines

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?

Quoted 6 lines

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.

Quoted 22 lines · Mark Rutland

[...]

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 9 lines

Quoted 4 lines

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)

Quoted 11 lines

Quoted 6 lines

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.

Quoted 22 lines · Mark Rutland

[...]

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 9 lines

Quoted 4 lines

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)

Quoted 8 lines

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 42 lines · Ard Biesheuvel

Quoted 22 lines · Mark Rutland

[...]

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 9 lines

Quoted 4 lines

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

Quoted 4 lines

"""
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.

Quoted 11 lines

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.

Quoted 20 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 4 lines

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.