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

PATCH New Ard Biesheuvel 2 participants 6 messages Jul 15, 2014

Subsystems (2)

arm64 EFI

lore

Share & tools

lore

Ard Biesheuvel

author

2014-07-15 09:10 UTC #0 lore raw

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

After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction.
However, the 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 <[email protected]>

---
 arch/arm64/kernel/efi-entry.S |  2 +-
 arch/arm64/kernel/head.S      | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

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

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

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

index a2c1195abb7f..78ddae28b090 100644
@@ -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)

Signature · 1 line

1.8.3.2

Mark Rutland

reviewer

2014-07-15 09:57 UTC #1 lore raw

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

Hi Ard,

Quoted 6 lines · Ard Biesheuvel

After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction.
However, the 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.

Quoted 11 lines

Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/efi-entry.S |  2 +-
 arch/arm64/kernel/head.S      | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)
arch/arm64/kernel/efi-entry.S +0 -0
index 619b1dd7bcde..6ef541731d9e 100644

arch/arm64/kernel/efi-entry.S ENTRY(efi_stub_entry) · line 64

Quoted 5 lines

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

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

Quoted 50 lines

arch/arm64/kernel/efi-entry.S +0 -0

 	/*
 	 * Flush dcache covering current runtime addresses

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

index a2c1195abb7f..78ddae28b090 100644
@@ -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)

Signature · 1 line

1.8.3.2

Ard Biesheuvel

author

2014-07-15 10:22 UTC #2 lore raw

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

Quoted 10 lines · Mark Rutland

Hi Ard,
Quoted 6 lines · Ard Biesheuvel
After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction.
However, the 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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/efi-entry.S |  2 +-
 arch/arm64/kernel/head.S      | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

Quoted 15 lines

arch/arm64/kernel/efi-entry.S ENTRY(efi_stub_entry) · line 64
Quoted 10 lines
arch/arm64/kernel/efi-entry.S +1 -1
index 619b1dd7bcde..6ef541731d9e 100644
@@ -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.

Quoted 3 lines

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

Cheers,
Ard.

Quoted 50 lines

arch/arm64/kernel/efi-entry.S +0 -0

      /*
       * Flush dcache covering current runtime addresses

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

index a2c1195abb7f..78ddae28b090 100644
@@ -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

Mark Rutland

reviewer

2014-07-15 11:31 UTC #3 lore raw

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

Quoted 16 lines

Quoted 6 lines
Quoted 6 lines
arch/arm64/kernel/efi-entry.S +0 -0
index 619b1dd7bcde..6ef541731d9e 100644
@@ -61,7 +61,7 @@ ENTRY(efi_stub_entry)
       */
Quoted 9 lines
arch/arm64/kernel/efi-entry.S ENTRY(efi_stub_entry) · line 64
Quoted 4 lines
arch/arm64/kernel/efi-entry.S +1 -1
      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?).

Quoted 2 lines

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.

Ard Biesheuvel

author

2014-07-15 11:49 UTC #4 lore raw

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

Quoted 16 lines · Mark Rutland

Quoted 16 lines
Quoted 15 lines
arch/arm64/kernel/efi-entry.S ENTRY(efi_stub_entry) · line 64
Quoted 10 lines
arch/arm64/kernel/efi-entry.S +1 -1
index 619b1dd7bcde..6ef541731d9e 100644
@@ -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?).

Quoted 6 lines

Quoted 2 lines
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

Quoted 3 lines

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)

Signature · 1 line

Ard.

Mark Rutland

reviewer

2014-07-15 12:44 UTC #5 lore raw

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

Quoted 32 lines · Ard Biesheuvel

Quoted 16 lines · Mark Rutland
Quoted 16 lines
Quoted 15 lines
arch/arm64/kernel/efi-entry.S ENTRY(efi_stub_entry) · line 64
Quoted 10 lines
arch/arm64/kernel/efi-entry.S +1 -1
index 619b1dd7bcde..6ef541731d9e 100644
@@ -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?).
Quoted 6 lines
Quoted 2 lines
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

Quoted 7 lines

Quoted 3 lines
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.

Quoted 1 line

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.