* [PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header
@ 2014-07-15 9:10 Ard Biesheuvel
2014-07-15 9:57 ` Mark Rutland
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2014-07-15 9:10 UTC (permalink / raw)
To: linux-arm-kernel
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@linaro.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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header
2014-07-15 9:10 [PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header Ard Biesheuvel
@ 2014-07-15 9:57 ` Mark Rutland
2014-07-15 10:22 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2014-07-15 9:57 UTC (permalink / raw)
To: linux-arm-kernel
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@linaro.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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header
2014-07-15 9:57 ` Mark Rutland
@ 2014-07-15 10:22 ` Ard Biesheuvel
2014-07-15 11:31 ` Mark Rutland
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2014-07-15 10:22 UTC (permalink / raw)
To: linux-arm-kernel
On 15 July 2014 11:57, Mark Rutland <mark.rutland@arm.com> 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@linaro.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
>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header
2014-07-15 10:22 ` Ard Biesheuvel
@ 2014-07-15 11:31 ` Mark Rutland
2014-07-15 11:49 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2014-07-15 11:31 UTC (permalink / raw)
To: linux-arm-kernel
> >> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header
2014-07-15 11:31 ` Mark Rutland
@ 2014-07-15 11:49 ` Ard Biesheuvel
2014-07-15 12:44 ` Mark Rutland
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2014-07-15 11:49 UTC (permalink / raw)
To: linux-arm-kernel
On 15 July 2014 13:31, Mark Rutland <mark.rutland@arm.com> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header
2014-07-15 11:49 ` Ard Biesheuvel
@ 2014-07-15 12:44 ` Mark Rutland
0 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2014-07-15 12:44 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 15, 2014 at 12:49:07PM +0100, Ard Biesheuvel wrote:
> On 15 July 2014 13:31, Mark Rutland <mark.rutland@arm.com> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-15 12:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-15 9:10 [PATCH] arm64/efi: efistub: jump to 'stext' directly, not through the header Ard Biesheuvel
2014-07-15 9:57 ` Mark Rutland
2014-07-15 10:22 ` Ard Biesheuvel
2014-07-15 11:31 ` Mark Rutland
2014-07-15 11:49 ` Ard Biesheuvel
2014-07-15 12:44 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).