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

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

[Ard Biesheuvel]

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

However, the header is not covered by any PE/COFF section, so the header may
not actually be loaded at the expected offset. 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>
---
 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 a2c1195abb7f..78ddae28b090 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -137,6 +137,8 @@ efi_head:
 #endif
 
 #ifdef CONFIG_EFI
+	.globl	stext_offset
+	.set	stext_offset, stext - efi_head
 	.align 3
 pe_header:
 	.ascii	"PE"
@@ -160,7 +162,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
@@ -177,7 +179,7 @@ extra_header_fields:
 	.long	_edata - 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
@@ -222,9 +224,9 @@ section_table:
 	.byte	0
 	.byte	0        		// end of 0 padding of section name
 	.long	_edata - 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

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

[Mark Salter]

On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
> not actually be loaded at the expected offset. 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'

Have you actually seen a situation where the header isn't there?
Isn't the kernel header actually part of the pe/coff file and
firmware loads the whole file into RAM?

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

[Mark Rutland]

On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
> On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
> > not actually be loaded at the expected offset. 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'
> 
> Have you actually seen a situation where the header isn't there?
> Isn't the kernel header actually part of the pe/coff file and
> firmware loads the whole file into RAM?

>From my understanding of Ard's earlier comments, this part isn't
guaranteed per the UEFI spec.

I would rather we weren't relying on implementation details.

Thanks,
Mark.

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

[Mark Salter]

On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
> > > not actually be loaded at the expected offset. 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'
> > 
> > Have you actually seen a situation where the header isn't there?
> > Isn't the kernel header actually part of the pe/coff file and
> > firmware loads the whole file into RAM?
> 
> From my understanding of Ard's earlier comments, this part isn't
> guaranteed per the UEFI spec.
> 
> I would rather we weren't relying on implementation details.
> 

Could be. I didn't see anything about it in the UEFI spec, but I
probably wasn't exhaustive in my search. In any case, there's at
least one other place broken if the kernel header isn't included
in the loaded image.

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

[Ard Biesheuvel]

On 16 July 2014 21:45, Mark Salter <msalter@redhat.com> wrote:
> On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
>> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
>> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
>> > > not actually be loaded at the expected offset. 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'
>> >
>> > Have you actually seen a situation where the header isn't there?
>> > Isn't the kernel header actually part of the pe/coff file and
>> > firmware loads the whole file into RAM?
>>
>> From my understanding of Ard's earlier comments, this part isn't
>> guaranteed per the UEFI spec.
>>
>> I would rather we weren't relying on implementation details.
>>
>
> Could be. I didn't see anything about it in the UEFI spec, but I
> probably wasn't exhaustive in my search. In any case, there's at
> least one other place broken if the kernel header isn't included
> in the loaded image.
>

I have not been able to find anything in the PE/COFF documents that
tells you what to put in memory areas that are not covered by a
section. Expecting the header to be there is indeed relying on an
implementation detail, which seems risky.
And indeed, if there are any other (non EFI related) uses of header
fields in the kernel, it would be good to have a look at those well,

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

[Mark Salter]

On Wed, 2014-07-16 at 22:38 +0200, Ard Biesheuvel wrote:
> On 16 July 2014 21:45, Mark Salter <msalter@redhat.com> wrote:
> > On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
> >> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
> >> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
> >> > > not actually be loaded at the expected offset. 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'
> >> >
> >> > Have you actually seen a situation where the header isn't there?
> >> > Isn't the kernel header actually part of the pe/coff file and
> >> > firmware loads the whole file into RAM?
> >>
> >> From my understanding of Ard's earlier comments, this part isn't
> >> guaranteed per the UEFI spec.
> >>
> >> I would rather we weren't relying on implementation details.
> >>
> >
> > Could be. I didn't see anything about it in the UEFI spec, but I
> > probably wasn't exhaustive in my search. In any case, there's at
> > least one other place broken if the kernel header isn't included
> > in the loaded image.
> >
> 
> I have not been able to find anything in the PE/COFF documents that
> tells you what to put in memory areas that are not covered by a
> section. Expecting the header to be there is indeed relying on an
> implementation detail, which seems risky.
> And indeed, if there are any other (non EFI related) uses of header
> fields in the kernel, it would be good to have a look at those well,

I think we need to come up with a loader which does load an image
without kernel header so that we can test. Otherwise, we'll probably
end up with buggy code anyway. The stub code assumes the the loaded
image pointed to by the system table is the whole image. Seems like
we'd need to add code to determine if it is whole kernel image or
image without initial header. Stub would have to handle both cases.
For instance, one case would want image placed at 2MiB+TEXT_OFFSET,
other case would want 2MiB+TEXT_OFFSET+sizeof(kernel header).

Am I just missing something here? Your "arm64/efi: efistub: get text
offset and image size from the Image header" patch makes no sense if we
can't rely on header being there.

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

[Ard Biesheuvel]

On 16 July 2014 23:03, Mark Salter <msalter@redhat.com> wrote:
> On Wed, 2014-07-16 at 22:38 +0200, Ard Biesheuvel wrote:
>> On 16 July 2014 21:45, Mark Salter <msalter@redhat.com> wrote:
>> > On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
>> >> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
>> >> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
>> >> > > not actually be loaded at the expected offset. 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'
>> >> >
>> >> > Have you actually seen a situation where the header isn't there?
>> >> > Isn't the kernel header actually part of the pe/coff file and
>> >> > firmware loads the whole file into RAM?
>> >>
>> >> From my understanding of Ard's earlier comments, this part isn't
>> >> guaranteed per the UEFI spec.
>> >>
>> >> I would rather we weren't relying on implementation details.
>> >>
>> >
>> > Could be. I didn't see anything about it in the UEFI spec, but I
>> > probably wasn't exhaustive in my search. In any case, there's at
>> > least one other place broken if the kernel header isn't included
>> > in the loaded image.
>> >
>>
>> I have not been able to find anything in the PE/COFF documents that
>> tells you what to put in memory areas that are not covered by a
>> section. Expecting the header to be there is indeed relying on an
>> implementation detail, which seems risky.
>> And indeed, if there are any other (non EFI related) uses of header
>> fields in the kernel, it would be good to have a look at those well,
>
> I think we need to come up with a loader which does load an image
> without kernel header so that we can test. Otherwise, we'll probably
> end up with buggy code anyway. The stub code assumes the the loaded
> image pointed to by the system table is the whole image. Seems like
> we'd need to add code to determine if it is whole kernel image or
> image without initial header. Stub would have to handle both cases.
> For instance, one case would want image placed at 2MiB+TEXT_OFFSET,
> other case would want 2MiB+TEXT_OFFSET+sizeof(kernel header).
>

No, this has nothing to do with misaligned data.

The PE/COFF .text section does not start at virtual offset #0 but at
virtual offset 'stext - efi_head'.
In other words, there is a hole in the virtual image where the header
is supposed to be.
So if there is no PE/COFF section describing what data should be put
at offset #0 by the loader, we can't assume the header is there, even
if ImageBase does start at #0

> Am I just missing something here? Your "arm64/efi: efistub: get text
> offset and image size from the Image header" patch makes no sense if we
> can't rely on header being there.
>

No, you're right, I reversed my position after doing that work and
digging into the details. I will drop those patches and proposed this
one instead.

-- 
Ard.

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

[Mark Salter]

On Wed, 2014-07-16 at 23:13 +0200, Ard Biesheuvel wrote:
> On 16 July 2014 23:03, Mark Salter <msalter@redhat.com> wrote:
> > On Wed, 2014-07-16 at 22:38 +0200, Ard Biesheuvel wrote:
> >> On 16 July 2014 21:45, Mark Salter <msalter@redhat.com> wrote:
> >> > On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
> >> >> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
> >> >> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
> >> >> > > not actually be loaded at the expected offset. 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'
> >> >> >
> >> >> > Have you actually seen a situation where the header isn't there?
> >> >> > Isn't the kernel header actually part of the pe/coff file and
> >> >> > firmware loads the whole file into RAM?
> >> >>
> >> >> From my understanding of Ard's earlier comments, this part isn't
> >> >> guaranteed per the UEFI spec.
> >> >>
> >> >> I would rather we weren't relying on implementation details.
> >> >>
> >> >
> >> > Could be. I didn't see anything about it in the UEFI spec, but I
> >> > probably wasn't exhaustive in my search. In any case, there's at
> >> > least one other place broken if the kernel header isn't included
> >> > in the loaded image.
> >> >
> >>
> >> I have not been able to find anything in the PE/COFF documents that
> >> tells you what to put in memory areas that are not covered by a
> >> section. Expecting the header to be there is indeed relying on an
> >> implementation detail, which seems risky.
> >> And indeed, if there are any other (non EFI related) uses of header
> >> fields in the kernel, it would be good to have a look at those well,
> >
> > I think we need to come up with a loader which does load an image
> > without kernel header so that we can test. Otherwise, we'll probably
> > end up with buggy code anyway. The stub code assumes the the loaded
> > image pointed to by the system table is the whole image. Seems like
> > we'd need to add code to determine if it is whole kernel image or
> > image without initial header. Stub would have to handle both cases.
> > For instance, one case would want image placed at 2MiB+TEXT_OFFSET,
> > other case would want 2MiB+TEXT_OFFSET+sizeof(kernel header).
> >
> 
> No, this has nothing to do with misaligned data.
> 
> The PE/COFF .text section does not start at virtual offset #0 but at
> virtual offset 'stext - efi_head'.
> In other words, there is a hole in the virtual image where the header
> is supposed to be.
> So if there is no PE/COFF section describing what data should be put
> at offset #0 by the loader, we can't assume the header is there, even
> if ImageBase does start at #0

I get that. You're supposing UEFI will always allocate memory for the
full image, but only sometimes copy the PE/COFF headers. I can see your
point from a PE/COFF perspective, but not so much from the UEFI spec
perspective where the language leads me to think it treats the PE/COFF
images as one unit wrt loading. In any case, it really isn't worth
arguing about. I don't have any objection to the patch since it won't
break anything from my perspective and it'll protect against breakage
which could possibly occur with some firmware implementations.

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

[Ard Biesheuvel]

On 17 July 2014 16:09, Mark Salter <msalter@redhat.com> wrote:
> On Wed, 2014-07-16 at 23:13 +0200, Ard Biesheuvel wrote:
>> On 16 July 2014 23:03, Mark Salter <msalter@redhat.com> wrote:
>> > On Wed, 2014-07-16 at 22:38 +0200, Ard Biesheuvel wrote:
>> >> On 16 July 2014 21:45, Mark Salter <msalter@redhat.com> wrote:
>> >> > On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
>> >> >> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
>> >> >> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
>> >> >> > > not actually be loaded at the expected offset. 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'
>> >> >> >
>> >> >> > Have you actually seen a situation where the header isn't there?
>> >> >> > Isn't the kernel header actually part of the pe/coff file and
>> >> >> > firmware loads the whole file into RAM?
>> >> >>
>> >> >> From my understanding of Ard's earlier comments, this part isn't
>> >> >> guaranteed per the UEFI spec.
>> >> >>
>> >> >> I would rather we weren't relying on implementation details.
>> >> >>
>> >> >
>> >> > Could be. I didn't see anything about it in the UEFI spec, but I
>> >> > probably wasn't exhaustive in my search. In any case, there's at
>> >> > least one other place broken if the kernel header isn't included
>> >> > in the loaded image.
>> >> >
>> >>
>> >> I have not been able to find anything in the PE/COFF documents that
>> >> tells you what to put in memory areas that are not covered by a
>> >> section. Expecting the header to be there is indeed relying on an
>> >> implementation detail, which seems risky.
>> >> And indeed, if there are any other (non EFI related) uses of header
>> >> fields in the kernel, it would be good to have a look at those well,
>> >
>> > I think we need to come up with a loader which does load an image
>> > without kernel header so that we can test. Otherwise, we'll probably
>> > end up with buggy code anyway. The stub code assumes the the loaded
>> > image pointed to by the system table is the whole image. Seems like
>> > we'd need to add code to determine if it is whole kernel image or
>> > image without initial header. Stub would have to handle both cases.
>> > For instance, one case would want image placed at 2MiB+TEXT_OFFSET,
>> > other case would want 2MiB+TEXT_OFFSET+sizeof(kernel header).
>> >
>>
>> No, this has nothing to do with misaligned data.
>>
>> The PE/COFF .text section does not start at virtual offset #0 but at
>> virtual offset 'stext - efi_head'.
>> In other words, there is a hole in the virtual image where the header
>> is supposed to be.
>> So if there is no PE/COFF section describing what data should be put
>> at offset #0 by the loader, we can't assume the header is there, even
>> if ImageBase does start at #0
>
> I get that. You're supposing UEFI will always allocate memory for the
> full image, but only sometimes copy the PE/COFF headers. I can see your
> point from a PE/COFF perspective, but not so much from the UEFI spec
> perspective where the language leads me to think it treats the PE/COFF
> images as one unit wrt loading. In any case, it really isn't worth
> arguing about. I don't have any objection to the patch since it won't
> break anything from my perspective and it'll protect against breakage
> which could possibly occur with some firmware implementations.
>

OK, thanks.

-- 
Ard.

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

[Ard Biesheuvel]

On 17 July 2014 16:09, Mark Salter <msalter@redhat.com> wrote:
> On Wed, 2014-07-16 at 23:13 +0200, Ard Biesheuvel wrote:
>> On 16 July 2014 23:03, Mark Salter <msalter@redhat.com> wrote:
>> > On Wed, 2014-07-16 at 22:38 +0200, Ard Biesheuvel wrote:
>> >> On 16 July 2014 21:45, Mark Salter <msalter@redhat.com> wrote:
>> >> > On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
>> >> >> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
>> >> >> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
>> >> >> > > not actually be loaded at the expected offset. 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'
>> >> >> >
>> >> >> > Have you actually seen a situation where the header isn't there?
>> >> >> > Isn't the kernel header actually part of the pe/coff file and
>> >> >> > firmware loads the whole file into RAM?
>> >> >>
>> >> >> From my understanding of Ard's earlier comments, this part isn't
>> >> >> guaranteed per the UEFI spec.
>> >> >>
>> >> >> I would rather we weren't relying on implementation details.
>> >> >>
>> >> >
>> >> > Could be. I didn't see anything about it in the UEFI spec, but I
>> >> > probably wasn't exhaustive in my search. In any case, there's at
>> >> > least one other place broken if the kernel header isn't included
>> >> > in the loaded image.
>> >> >
>> >>
>> >> I have not been able to find anything in the PE/COFF documents that
>> >> tells you what to put in memory areas that are not covered by a
>> >> section. Expecting the header to be there is indeed relying on an
>> >> implementation detail, which seems risky.
>> >> And indeed, if there are any other (non EFI related) uses of header
>> >> fields in the kernel, it would be good to have a look at those well,
>> >
>> > I think we need to come up with a loader which does load an image
>> > without kernel header so that we can test. Otherwise, we'll probably
>> > end up with buggy code anyway. The stub code assumes the the loaded
>> > image pointed to by the system table is the whole image. Seems like
>> > we'd need to add code to determine if it is whole kernel image or
>> > image without initial header. Stub would have to handle both cases.
>> > For instance, one case would want image placed at 2MiB+TEXT_OFFSET,
>> > other case would want 2MiB+TEXT_OFFSET+sizeof(kernel header).
>> >
>>
>> No, this has nothing to do with misaligned data.
>>
>> The PE/COFF .text section does not start at virtual offset #0 but at
>> virtual offset 'stext - efi_head'.
>> In other words, there is a hole in the virtual image where the header
>> is supposed to be.
>> So if there is no PE/COFF section describing what data should be put
>> at offset #0 by the loader, we can't assume the header is there, even
>> if ImageBase does start at #0
>
> I get that. You're supposing UEFI will always allocate memory for the
> full image, but only sometimes copy the PE/COFF headers. I can see your
> point from a PE/COFF perspective, but not so much from the UEFI spec
> perspective where the language leads me to think it treats the PE/COFF
> images as one unit wrt loading. In any case, it really isn't worth
> arguing about. I don't have any objection to the patch since it won't
> break anything from my perspective and it'll protect against breakage
> which could possibly occur with some firmware implementations.
>

I am reviving this old thread because it appears we may have seen an
issue involving shim and GRUB where data not covered by any loadable
PE/COFF section was not actually loaded to memory. In this case, it
was the ".reloc" section, not the header but the conclusion should be
the same.

@Peter: this is a second-hand account so perhaps you could fill in
with some details? Original thread is here:
http://marc.info/?l=linux-arm-kernel&m=140542202520933&w=2

-- 
Ard.

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

[Peter Jones]

On Mon, Oct 06, 2014 at 08:13:01PM +0200, Ard Biesheuvel wrote:
> On 17 July 2014 16:09, Mark Salter <msalter@redhat.com> wrote:
> > On Wed, 2014-07-16 at 23:13 +0200, Ard Biesheuvel wrote:
> >> On 16 July 2014 23:03, Mark Salter <msalter@redhat.com> wrote:
> >> > On Wed, 2014-07-16 at 22:38 +0200, Ard Biesheuvel wrote:
> >> >> On 16 July 2014 21:45, Mark Salter <msalter@redhat.com> wrote:
> >> >> > On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
> >> >> >> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
> >> >> >> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
> >> >> >> > > not actually be loaded at the expected offset. 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'
> >> >> >> >
> >> >> >> > Have you actually seen a situation where the header isn't there?
> >> >> >> > Isn't the kernel header actually part of the pe/coff file and
> >> >> >> > firmware loads the whole file into RAM?
> >> >> >>
> >> >> >> From my understanding of Ard's earlier comments, this part isn't
> >> >> >> guaranteed per the UEFI spec.
> >> >> >>
> >> >> >> I would rather we weren't relying on implementation details.
> >> >> >>
> >> >> >
> >> >> > Could be. I didn't see anything about it in the UEFI spec, but I
> >> >> > probably wasn't exhaustive in my search. In any case, there's at
> >> >> > least one other place broken if the kernel header isn't included
> >> >> > in the loaded image.
> >> >> >
> >> >>
> >> >> I have not been able to find anything in the PE/COFF documents that
> >> >> tells you what to put in memory areas that are not covered by a
> >> >> section. Expecting the header to be there is indeed relying on an
> >> >> implementation detail, which seems risky.
> >> >> And indeed, if there are any other (non EFI related) uses of header
> >> >> fields in the kernel, it would be good to have a look at those well,
> >> >
> >> > I think we need to come up with a loader which does load an image
> >> > without kernel header so that we can test. Otherwise, we'll probably
> >> > end up with buggy code anyway. The stub code assumes the the loaded
> >> > image pointed to by the system table is the whole image. Seems like
> >> > we'd need to add code to determine if it is whole kernel image or
> >> > image without initial header. Stub would have to handle both cases.
> >> > For instance, one case would want image placed at 2MiB+TEXT_OFFSET,
> >> > other case would want 2MiB+TEXT_OFFSET+sizeof(kernel header).
> >> >
> >>
> >> No, this has nothing to do with misaligned data.
> >>
> >> The PE/COFF .text section does not start at virtual offset #0 but at
> >> virtual offset 'stext - efi_head'.
> >> In other words, there is a hole in the virtual image where the header
> >> is supposed to be.
> >> So if there is no PE/COFF section describing what data should be put
> >> at offset #0 by the loader, we can't assume the header is there, even
> >> if ImageBase does start at #0
> >
> > I get that. You're supposing UEFI will always allocate memory for the
> > full image, but only sometimes copy the PE/COFF headers. I can see your
> > point from a PE/COFF perspective, but not so much from the UEFI spec
> > perspective where the language leads me to think it treats the PE/COFF
> > images as one unit wrt loading. In any case, it really isn't worth
> > arguing about. I don't have any objection to the patch since it won't
> > break anything from my perspective and it'll protect against breakage
> > which could possibly occur with some firmware implementations.
> >
> 
> I am reviving this old thread because it appears we may have seen an
> issue involving shim and GRUB where data not covered by any loadable
> PE/COFF section was not actually loaded to memory. In this case, it
> was the ".reloc" section, not the header but the conclusion should be
> the same.
> 
> @Peter: this is a second-hand account so perhaps you could fill in
> with some details? Original thread is here:
> http://marc.info/?l=linux-arm-kernel&m=140542202520933&w=2

So what happened with the shim+grub .reloc problem was that grub's
binary has relocations (which I think don't strictly need to be
processed), but shim's relocation code was *completely* defective.

Then I fixed shim to try to process relocations, but got it wrong
because in my mind data directories were file addresses rather than
relative virtual addresses (often they are identical, but by spec
they're RVAs). In grub's binary, everything has matching file addresses
and RVAs.  So grub worked, but other things did not.  That change is
here:

https://github.com/mjg59/shim/commit/a846aedd0e9dfe26ca6afaf6a1db8a54c20363c1

Then I realized that they are always RVAs and we have to match the base
relocation data directory up with the .reloc section for processing.  So
in the current code we /do/ load .reloc into memory to process the
relocations if we find the section there, but we don't mirror it into
the final image we've relocated and are executing if it's marked
discardable, which it typically is.

That change is here:

https://github.com/mjg59/shim/commit/a846aedd0e9dfe26ca6afaf6a1db8a54c20363c1

And it seems to work with all the binaries we actually generate,
including grub, MokManager.efi, fallback.efi, etc.

Just in general, though, I don't see that there's any PE spec
requirement that we have to copy anything from the binary into the
address range we're going to jump in to unless all of the following are
true:

1) it is covered by a section header
2) the section is non-discardable
3) the section's SizeOfRawData is nonzero
4) basic constraints of sanity are met - i.e. the section data is
   actually entirely within the binary, it's doesn't overlap with the
   data directory or the PE headers, etc.

There is the obvious case where something should exist in your running
copy that is not not covered by the above - .bss and such.  But those
are not /copied/ from the binary.

So is there a question now?

-- 
        Peter

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

[Ard Biesheuvel]

On 6 October 2014 21:33, Peter Jones <pjones@redhat.com> wrote:
> On Mon, Oct 06, 2014 at 08:13:01PM +0200, Ard Biesheuvel wrote:
>> On 17 July 2014 16:09, Mark Salter <msalter@redhat.com> wrote:
>> > On Wed, 2014-07-16 at 23:13 +0200, Ard Biesheuvel wrote:
>> >> On 16 July 2014 23:03, Mark Salter <msalter@redhat.com> wrote:
>> >> > On Wed, 2014-07-16 at 22:38 +0200, Ard Biesheuvel wrote:
>> >> >> On 16 July 2014 21:45, Mark Salter <msalter@redhat.com> wrote:
>> >> >> > On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
>> >> >> >> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
>> >> >> >> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
>> >> >> >> > > not actually be loaded at the expected offset. 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'
>> >> >> >> >
>> >> >> >> > Have you actually seen a situation where the header isn't there?
>> >> >> >> > Isn't the kernel header actually part of the pe/coff file and
>> >> >> >> > firmware loads the whole file into RAM?
>> >> >> >>
>> >> >> >> From my understanding of Ard's earlier comments, this part isn't
>> >> >> >> guaranteed per the UEFI spec.
>> >> >> >>
>> >> >> >> I would rather we weren't relying on implementation details.
>> >> >> >>
>> >> >> >
>> >> >> > Could be. I didn't see anything about it in the UEFI spec, but I
>> >> >> > probably wasn't exhaustive in my search. In any case, there's at
>> >> >> > least one other place broken if the kernel header isn't included
>> >> >> > in the loaded image.
>> >> >> >
>> >> >>
>> >> >> I have not been able to find anything in the PE/COFF documents that
>> >> >> tells you what to put in memory areas that are not covered by a
>> >> >> section. Expecting the header to be there is indeed relying on an
>> >> >> implementation detail, which seems risky.
>> >> >> And indeed, if there are any other (non EFI related) uses of header
>> >> >> fields in the kernel, it would be good to have a look at those well,
>> >> >
>> >> > I think we need to come up with a loader which does load an image
>> >> > without kernel header so that we can test. Otherwise, we'll probably
>> >> > end up with buggy code anyway. The stub code assumes the the loaded
>> >> > image pointed to by the system table is the whole image. Seems like
>> >> > we'd need to add code to determine if it is whole kernel image or
>> >> > image without initial header. Stub would have to handle both cases.
>> >> > For instance, one case would want image placed at 2MiB+TEXT_OFFSET,
>> >> > other case would want 2MiB+TEXT_OFFSET+sizeof(kernel header).
>> >> >
>> >>
>> >> No, this has nothing to do with misaligned data.
>> >>
>> >> The PE/COFF .text section does not start at virtual offset #0 but at
>> >> virtual offset 'stext - efi_head'.
>> >> In other words, there is a hole in the virtual image where the header
>> >> is supposed to be.
>> >> So if there is no PE/COFF section describing what data should be put
>> >> at offset #0 by the loader, we can't assume the header is there, even
>> >> if ImageBase does start at #0
>> >
>> > I get that. You're supposing UEFI will always allocate memory for the
>> > full image, but only sometimes copy the PE/COFF headers. I can see your
>> > point from a PE/COFF perspective, but not so much from the UEFI spec
>> > perspective where the language leads me to think it treats the PE/COFF
>> > images as one unit wrt loading. In any case, it really isn't worth
>> > arguing about. I don't have any objection to the patch since it won't
>> > break anything from my perspective and it'll protect against breakage
>> > which could possibly occur with some firmware implementations.
>> >
>>
>> I am reviving this old thread because it appears we may have seen an
>> issue involving shim and GRUB where data not covered by any loadable
>> PE/COFF section was not actually loaded to memory. In this case, it
>> was the ".reloc" section, not the header but the conclusion should be
>> the same.
>>
>> @Peter: this is a second-hand account so perhaps you could fill in
>> with some details? Original thread is here:
>> http://marc.info/?l=linux-arm-kernel&m=140542202520933&w=2
>
> So what happened with the shim+grub .reloc problem was that grub's
> binary has relocations (which I think don't strictly need to be
> processed), but shim's relocation code was *completely* defective.
>
> Then I fixed shim to try to process relocations, but got it wrong
> because in my mind data directories were file addresses rather than
> relative virtual addresses (often they are identical, but by spec
> they're RVAs). In grub's binary, everything has matching file addresses
> and RVAs.  So grub worked, but other things did not.  That change is
> here:
>
> https://github.com/mjg59/shim/commit/a846aedd0e9dfe26ca6afaf6a1db8a54c20363c1
>
> Then I realized that they are always RVAs and we have to match the base
> relocation data directory up with the .reloc section for processing.  So
> in the current code we /do/ load .reloc into memory to process the
> relocations if we find the section there, but we don't mirror it into
> the final image we've relocated and are executing if it's marked
> discardable, which it typically is.
>
> That change is here:
>
> https://github.com/mjg59/shim/commit/a846aedd0e9dfe26ca6afaf6a1db8a54c20363c1
>
> And it seems to work with all the binaries we actually generate,
> including grub, MokManager.efi, fallback.efi, etc.
>
> Just in general, though, I don't see that there's any PE spec
> requirement that we have to copy anything from the binary into the
> address range we're going to jump in to unless all of the following are
> true:
>
> 1) it is covered by a section header
> 2) the section is non-discardable
> 3) the section's SizeOfRawData is nonzero
> 4) basic constraints of sanity are met - i.e. the section data is
>    actually entirely within the binary, it's doesn't overlap with the
>    data directory or the PE headers, etc.
>
> There is the obvious case where something should exist in your running
> copy that is not not covered by the above - .bss and such.  But those
> are not /copied/ from the binary.
>
> So is there a question now?
>

You have answered my question: it confirms that it is a bad idea to
rely on the header having been copied to memory at the same relative
offset from .text as it appears in the file.

Thanks for the elaborate response. I will respin the patch against
3.17, test and repost.

-- 
Ard.

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

[Roy Franz]

On Wed, Jul 16, 2014 at 1:38 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 16 July 2014 21:45, Mark Salter <msalter@redhat.com> wrote:
>> On Wed, 2014-07-16 at 16:53 +0100, Mark Rutland wrote:
>>> On Wed, Jul 16, 2014 at 03:51:37PM +0100, Mark Salter wrote:
>>> > On Tue, 2014-07-15 at 12:58 +0200, 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 header is not covered by any PE/COFF section, so the header may
>>> > > not actually be loaded at the expected offset. 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'
>>> >
>>> > Have you actually seen a situation where the header isn't there?
>>> > Isn't the kernel header actually part of the pe/coff file and
>>> > firmware loads the whole file into RAM?
>>>
>>> From my understanding of Ard's earlier comments, this part isn't
>>> guaranteed per the UEFI spec.
>>>
>>> I would rather we weren't relying on implementation details.
>>>
>>
>> Could be. I didn't see anything about it in the UEFI spec, but I
>> probably wasn't exhaustive in my search. In any case, there's at
>> least one other place broken if the kernel header isn't included
>> in the loaded image.
>>
>
> I have not been able to find anything in the PE/COFF documents that
> tells you what to put in memory areas that are not covered by a
> section. Expecting the header to be there is indeed relying on an
> implementation detail, which seems risky.
> And indeed, if there are any other (non EFI related) uses of header
> fields in the kernel, it would be good to have a look at those well,

I looked through the UEFI spec, and didn't see anything really helpful
in this regard.  However the "ImageBase" address that is returned
by the loadedImageProtocol is the address of the header, and this
address is aligned as specified in the PE/COFF header.  It would
be strange to specify the alignment of something that is not expected
to be loaded.
It's probably worth asking on the edk2 list regarding what the UEFI
specification requires.

Roy