| Message ID | 1412777487-13636-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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@linaro.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 > >
On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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.
On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote: > On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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. >
On Thu, Oct 9, 2014 at 3:19 PM, Mark Salter <msalter@redhat.com> wrote: > On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote: >> On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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. >> > >
On 10 October 2014 01:20, Roy Franz <roy.franz@linaro.org> wrote: > On Thu, Oct 9, 2014 at 3:19 PM, Mark Salter <msalter@redhat.com> wrote: >> On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote: >>> On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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.
On Thu, Oct 09, 2014 at 08:03:52PM +0100, Ard Biesheuvel wrote: > On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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.
On 10 October 2014 12:49, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Oct 09, 2014 at 08:03:52PM +0100, Ard Biesheuvel wrote: >> On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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.
On Fri, Oct 10, 2014 at 12:52:32PM +0100, Ard Biesheuvel wrote: > On 10 October 2014 12:49, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Oct 09, 2014 at 08:03:52PM +0100, Ard Biesheuvel wrote: > >> On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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.
On 10 October 2014 14:19, Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Oct 10, 2014 at 12:52:32PM +0100, Ard Biesheuvel wrote: >> On 10 October 2014 12:49, Mark Rutland <mark.rutland@arm.com> wrote: >> > On Thu, Oct 09, 2014 at 08:03:52PM +0100, Ard Biesheuvel wrote: >> >> On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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.
[...] > >> > 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.
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. """ 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.
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.
On Fri, Oct 10, 2014 at 02:27:46PM +0100, Ard Biesheuvel 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. > """ 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.
On Fri, 2014-10-10 at 08:30 +0200, Ard Biesheuvel wrote: > On 10 October 2014 01:20, Roy Franz <roy.franz@linaro.org> wrote: > > On Thu, Oct 9, 2014 at 3:19 PM, Mark Salter <msalter@redhat.com> wrote: > >> On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote: > >>> On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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.
On 10 October 2014 16:14, Mark Salter <msalter@redhat.com> wrote: > On Fri, 2014-10-10 at 08:30 +0200, Ard Biesheuvel wrote: >> On 10 October 2014 01:20, Roy Franz <roy.franz@linaro.org> wrote: >> > On Thu, Oct 9, 2014 at 3:19 PM, Mark Salter <msalter@redhat.com> wrote: >> >> On Thu, 2014-10-09 at 21:03 +0200, Ard Biesheuvel wrote: >> >>> On 9 October 2014 19:23, Mark Rutland <mark.rutland@arm.com> 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.
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. > > 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.
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.
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)
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@linaro.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(-)