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

[PATCH] 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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm64/kernel/efi-entry.S |  2 +-
 arch/arm64/kernel/head.S      | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
index 619b1dd7bcde..6ef541731d9e 100644
--- a/arch/arm64/kernel/efi-entry.S
+++ b/arch/arm64/kernel/efi-entry.S
@@ -61,7 +61,7 @@ ENTRY(efi_stub_entry)
 	 */
 	mov	x20, x0		// DTB address
 	ldr	x0, [sp, #16]	// relocated _text address
-	mov	x21, x0
+	add	x21, x0, #:lo12:stext_offset
 
 	/*
 	 * 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

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

[Mark Rutland]

Hi Ard,

On Tue, Jul 15, 2014 at 10:10:02AM +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 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.

It would be nice to point out in the commit message that the other
changes in the patch are just cleanup to use stext_offset rather than
open-coding it.

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm64/kernel/efi-entry.S |  2 +-
>  arch/arm64/kernel/head.S      | 10 ++++++----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
> index 619b1dd7bcde..6ef541731d9e 100644
> --- a/arch/arm64/kernel/efi-entry.S
> +++ b/arch/arm64/kernel/efi-entry.S
> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry)
>  	 */
>  	mov	x20, x0		// DTB address
>  	ldr	x0, [sp, #16]	// relocated _text address
> -	mov	x21, x0
> +	add	x21, x0, #:lo12:stext_offset

I think we can drop the :lo12: here, which will allow us to have a
warning if stext_offset is unexpectedly large (I believe this will
currently silently mask bits were that to happen?).

Other than that, this looks like a sensible thing to do given that we
cannot rely on the header being present.

Cheers,
Mark.

>  
>  	/*
>  	 * 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
> 
> 

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

[Ard Biesheuvel]

On 15 July 2014 11:57, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Ard,
>
> On Tue, Jul 15, 2014 at 10:10:02AM +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 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.
>
> It would be nice to point out in the commit message that the other
> changes in the patch are just cleanup to use stext_offset rather than
> open-coding it.
>

OK

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  arch/arm64/kernel/efi-entry.S |  2 +-
>>  arch/arm64/kernel/head.S      | 10 ++++++----
>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
>> index 619b1dd7bcde..6ef541731d9e 100644
>> --- a/arch/arm64/kernel/efi-entry.S
>> +++ b/arch/arm64/kernel/efi-entry.S
>> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry)
>>        */
>>       mov     x20, x0         // DTB address
>>       ldr     x0, [sp, #16]   // relocated _text address
>> -     mov     x21, x0
>> +     add     x21, x0, #:lo12:stext_offset
>
> I think we can drop the :lo12: here, which will allow us to have a
> warning if stext_offset is unexpectedly large (I believe this will
> currently silently mask bits were that to happen?).
>

There is no alternative lo12 relocation that errors out when the value
does not fit, so it would have to use a literal instead.

> Other than that, this looks like a sensible thing to do given that we
> cannot rely on the header being present.
>

Cheers,
Ard.


>>
>>       /*
>>        * 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
>>
>>

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

[Mark Rutland]

> >> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
> >> index 619b1dd7bcde..6ef541731d9e 100644
> >> --- a/arch/arm64/kernel/efi-entry.S
> >> +++ b/arch/arm64/kernel/efi-entry.S
> >> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry)
> >>        */
> >>       mov     x20, x0         // DTB address
> >>       ldr     x0, [sp, #16]   // relocated _text address
> >> -     mov     x21, x0
> >> +     add     x21, x0, #:lo12:stext_offset
> >
> > I think we can drop the :lo12: here, which will allow us to have a
> > warning if stext_offset is unexpectedly large (I believe this will
> > currently silently mask bits were that to happen?).
> >
> 
> There is no alternative lo12 relocation that errors out when the value
> does not fit, so it would have to use a literal instead.

Ah, that's a shame. What happens when the value doesn't fit if the
linker / assembler don't error out?

That sounds like a toolchain bug if they're silently doing the wrong
thing.

Cheers,
Mark.

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

[Ard Biesheuvel]

On 15 July 2014 13:31, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> >> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
>> >> index 619b1dd7bcde..6ef541731d9e 100644
>> >> --- a/arch/arm64/kernel/efi-entry.S
>> >> +++ b/arch/arm64/kernel/efi-entry.S
>> >> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry)
>> >>        */
>> >>       mov     x20, x0         // DTB address
>> >>       ldr     x0, [sp, #16]   // relocated _text address
>> >> -     mov     x21, x0
>> >> +     add     x21, x0, #:lo12:stext_offset
>> >
>> > I think we can drop the :lo12: here, which will allow us to have a
>> > warning if stext_offset is unexpectedly large (I believe this will
>> > currently silently mask bits were that to happen?).
>> >
>>
>> There is no alternative lo12 relocation that errors out when the value
>> does not fit, so it would have to use a literal instead.
>
> Ah, that's a shame. What happens when the value doesn't fit if the
> linker / assembler don't error out?
>

Obviously, it will jump into the wrong place if stext ever gets pushed
beyond a 4k boundary, which is not that likely (current value is
0x160, we may want to up it to 0x200 at some point if we need to
increase the PE/COFF section alignment, which some versions of the
(poor) PE/COFF documentation say should be 0x200 at the minimum)

However, doing ldr x21, =stext_offset works fine as well

> That sounds like a toolchain bug if they're silently doing the wrong
> thing.
>

Meh, don't think so, really. You get what you pay for, and :lo12: just
isn't supposed to care. (It's mainly used with adrp to get a +/- 4gb
PC relative reach)

-- 
Ard.

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

[Mark Rutland]

On Tue, Jul 15, 2014 at 12:49:07PM +0100, Ard Biesheuvel wrote:
> On 15 July 2014 13:31, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> >> >> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
> >> >> index 619b1dd7bcde..6ef541731d9e 100644
> >> >> --- a/arch/arm64/kernel/efi-entry.S
> >> >> +++ b/arch/arm64/kernel/efi-entry.S
> >> >> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry)
> >> >>        */
> >> >>       mov     x20, x0         // DTB address
> >> >>       ldr     x0, [sp, #16]   // relocated _text address
> >> >> -     mov     x21, x0
> >> >> +     add     x21, x0, #:lo12:stext_offset
> >> >
> >> > I think we can drop the :lo12: here, which will allow us to have a
> >> > warning if stext_offset is unexpectedly large (I believe this will
> >> > currently silently mask bits were that to happen?).
> >> >
> >>
> >> There is no alternative lo12 relocation that errors out when the value
> >> does not fit, so it would have to use a literal instead.
> >
> > Ah, that's a shame. What happens when the value doesn't fit if the
> > linker / assembler don't error out?
> >
> 
> Obviously, it will jump into the wrong place if stext ever gets pushed
> beyond a 4k boundary, which is not that likely (current value is
> 0x160, we may want to up it to 0x200 at some point if we need to
> increase the PE/COFF section alignment, which some versions of the
> (poor) PE/COFF documentation say should be 0x200 at the minimum)
> 
> However, doing ldr x21, =stext_offset works fine as well
> 
> > That sounds like a toolchain bug if they're silently doing the wrong
> > thing.
> >
> 
> Meh, don't think so, really. You get what you pay for, and :lo12: just
> isn't supposed to care. (It's mainly used with adrp to get a +/- 4gb
> PC relative reach)

We appear to have a misunderstanding.

I was suggesting we use:

	add	x21, x0, #stext_offset

... and I thought you meant that wouldn't produce an error if the
immediate was out of range. I understand that :lo12: cuts the top bits
off, and for its intended use that makes sense.

>From testing I see see that gas isn't happy to accept an undefined
symbol as an immediate without :lo12:, and I can see why that makes
sense. My suggestion was bad, sorry.

Thanks,
Mark.