arm64/efi: set PE/COFF section alignment to 4 KB

PATCH New Ard Biesheuvel 3 participants 8 messages Oct 10, 2014

Subsystems (2)

arm64 EFI

lore

Share & tools

lore

Ard Biesheuvel

author

2014-10-10 09:25 UTC #0 lore raw

PATCH 1/1 arm64/efi: set PE/COFF section alignment to 4 KB

Position independent AArch64 code needs to be linked and loaded at the same relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will not work correctly. (This is how PC relative symbol references with a 4 GB reach are emitted)

We need to declare this in the PE/COFF header, otherwise the PE/COFF loader may load the Image and invoke the stub at an offset which violates this rule.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/head.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/kernel/head.S +2 -2

index 0a6e4f924df8..5e83e5b8a9de 100644
@@ -159,7 +159,7 @@ optional_header:
 
 extra_header_fields:
 	.quad	0				// ImageBase
-	.long	0x20				// SectionAlignment
+	.long	0x1000				// SectionAlignment
 	.long	0x8				// FileAlignment
 	.short	0				// MajorOperatingSystemVersion
 	.short	0				// MinorOperatingSystemVersion

@@ -226,7 +226,7 @@ section_table:
 	.short	0		// NumberOfRelocations  (0 for executables)
 	.short	0		// NumberOfLineNumbers  (0 for executables)
 	.long	0xe0500020	// Characteristics (section flags)
-	.align 5
+	.align 12
 #endif
 
 ENTRY(stext)

Signature · 1 line

1.8.3.2

Mark Rutland

reviewer

2014-10-10 10:33 UTC #1 lore raw

PATCH 1/1 arm64/efi: set PE/COFF section alignment to 4 KB

Hi Ard,

Quoted 7 lines · Ard Biesheuvel

Position independent AArch64 code needs to be linked and loaded at the same relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will not work correctly. (This is how PC relative symbol references with a 4 GB reach are emitted)
We need to declare this in the PE/COFF header, otherwise the PE/COFF loader may load the Image and invoke the stub at an offset which violates this rule.

Has this been observed happening, or was this just found by inspection?

Quoted 19 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/head.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/kernel/head.S +1 -1

index 0a6e4f924df8..5e83e5b8a9de 100644
@@ -159,7 +159,7 @@ optional_header:
 
 extra_header_fields:
 	.quad	0				// ImageBase
-	.long	0x20				// SectionAlignment
+	.long	0x1000				// SectionAlignment
 	.long	0x8				// FileAlignment
 	.short	0				// MajorOperatingSystemVersion
 	.short	0				// MinorOperatingSystemVersion

arch/arm64/kernel/head.S section_table: · line 229

Quoted 5 lines

arch/arm64/kernel/head.S +1 -1

@@ -226,7 +226,7 @@ section_table:
 	.short	0		// NumberOfRelocations  (0 for executables)
 	.short	0		// NumberOfLineNumbers  (0 for executables)
 	.long	0xe0500020	// Characteristics (section flags)
-	.align 5
+	.align 12

Can we get a comment explaining why stext needs the additional alignment? Something like:

/*
* EFI will load stext onwards at the 4k section alignment
* described in the PE/COFF header. To ensure that instruction
* sequences using an adrp and a :lo12: immediate will function
* correctly at this alignment, we must ensure that stext is
* placed at a 4k boundary in the Image to begin with. */
.align 12

Otherwise this looks sane to me.

Thanks,
Mark.

Ard Biesheuvel

author

2014-10-10 10:37 UTC #2 lore raw

PATCH 1/1 arm64/efi: set PE/COFF section alignment to 4 KB

Quoted 8 lines · Mark Rutland

Hi Ard,
Quoted 5 lines · Ard Biesheuvel
Position independent AArch64 code needs to be linked and loaded at the same relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will not work correctly. (This is how PC relative symbol references with a 4 GB reach are emitted)

Quoted 5 lines

Quoted 2 lines
We need to declare this in the PE/COFF header, otherwise the PE/COFF loader may load the Image and invoke the stub at an offset which violates this rule.
Has this been observed happening, or was this just found by inspection?

This is also something found by inspection, or rather, by the discussion going on in the other thread. I am not aware of any PE/COFF loaders that may choose an offset that is not 4 KB aligned, even if the header we give it appears to allow it.

Quoted 28 lines

Quoted 19 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/head.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/kernel/head.S +1 -1

index 0a6e4f924df8..5e83e5b8a9de 100644
@@ -159,7 +159,7 @@ optional_header:
 
 extra_header_fields:
      .quad   0                               // ImageBase
-     .long   0x20                            // SectionAlignment
+     .long   0x1000                          // SectionAlignment
      .long   0x8                             // FileAlignment
      .short  0                               // MajorOperatingSystemVersion
      .short  0                               // MinorOperatingSystemVersion

arch/arm64/kernel/head.S section_table: · line 229

Quoted 5 lines

arch/arm64/kernel/head.S +1 -1

@@ -226,7 +226,7 @@ section_table:
      .short  0               // NumberOfRelocations  (0 for executables)
      .short  0               // NumberOfLineNumbers  (0 for executables)
      .long   0xe0500020      // Characteristics (section flags)
-     .align 5
+     .align 12

Can we get a comment explaining why stext needs the additional alignment? Something like:

Quoted 9 lines

/*
* EFI will load stext onwards at the 4k section alignment
* described in the PE/COFF header. To ensure that instruction
* sequences using an adrp and a :lo12: immediate will function
* correctly at this alignment, we must ensure that stext is
* placed at a 4k boundary in the Image to begin with. */
.align 12

Signature · 1 line

Ard.

Mark Rutland

reviewer

2014-10-10 14:09 UTC #3 lore raw

PATCH 1/1 arm64/efi: set PE/COFF section alignment to 4 KB

Quoted 31 lines · Ard Biesheuvel

Quoted 8 lines · Mark Rutland
Hi Ard,
Quoted 5 lines · Ard Biesheuvel
Position independent AArch64 code needs to be linked and loaded at the same relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will not work correctly. (This is how PC relative symbol references with a 4 GB reach are emitted)
Quoted 5 lines
Quoted 2 lines
We need to declare this in the PE/COFF header, otherwise the PE/COFF loader may load the Image and invoke the stub at an offset which violates this rule.
Has this been observed happening, or was this just found by inspection?
This is also something found by inspection, or rather, by the discussion going on in the other thread. I am not aware of any PE/COFF loaders that may choose an offset that is not 4 KB aligned, even if the header we give it appears to allow it.
Quoted 11 lines
Quoted 11 lines
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/head.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm64/kernel/head.S +0 -0
index 0a6e4f924df8..5e83e5b8a9de 100644
@@ -159,7 +159,7 @@ optional_header:
 

Quoted 4 lines

Quoted 4 lines
arch/arm64/kernel/head.S optional_header: · line 162
Quoted 4 lines
arch/arm64/kernel/head.S +1 -1
 extra_header_fields:
      .quad   0                               // ImageBase
-     .long   0x20                            // SectionAlignment
+     .long   0x1000                          // SectionAlignment

Looking at this again, I'm more confused than I was to begin with.

Surely we know exactly where the .text section will be loaded because of its VirtualAddress? If that were the case, we can drop the .align 12 as we already load it at the offset any arp or :lo12: immediate will have been built for.

I don't follow how SectionAlignment interacts with a given section's VirtualAddress.

Thanks,
Mark.

Quoted 28 lines

Quoted 13 lines
arch/arm64/kernel/head.S section_table: · line 229
Quoted 9 lines
arch/arm64/kernel/head.S +1 -1
      .long   0x8                             // FileAlignment
      .short  0                               // MajorOperatingSystemVersion
      .short  0                               // MinorOperatingSystemVersion
@@ -226,7 +226,7 @@ section_table:
      .short  0               // NumberOfRelocations  (0 for executables)
      .short  0               // NumberOfLineNumbers  (0 for executables)
      .long   0xe0500020      // Characteristics (section flags)
-     .align 5
+     .align 12
Can we get a comment explaining why stext needs the additional alignment? Something like:
Quoted 9 lines
/*
* EFI will load stext onwards at the 4k section alignment
* described in the PE/COFF header. To ensure that instruction
* sequences using an adrp and a :lo12: immediate will function
* correctly at this alignment, we must ensure that stext is
* placed at a 4k boundary in the Image to begin with. */
.align 12
OK
Signature · 1 line
Ard.

Ard Biesheuvel

author

2014-10-10 14:50 UTC #4 lore raw

PATCH 1/1 arm64/efi: set PE/COFF section alignment to 4 KB

Quoted 32 lines · Mark Rutland

Quoted 31 lines · Ard Biesheuvel
Quoted 8 lines · Mark Rutland
Hi Ard,
Quoted 5 lines · Ard Biesheuvel
Position independent AArch64 code needs to be linked and loaded at the same relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will not work correctly. (This is how PC relative symbol references with a 4 GB reach are emitted)
Quoted 5 lines
Quoted 2 lines
We need to declare this in the PE/COFF header, otherwise the PE/COFF loader may load the Image and invoke the stub at an offset which violates this rule.
Has this been observed happening, or was this just found by inspection?
This is also something found by inspection, or rather, by the discussion going on in the other thread. I am not aware of any PE/COFF loaders that may choose an offset that is not 4 KB aligned, even if the header we give it appears to allow it.
Quoted 11 lines
Quoted 11 lines
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/head.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm64/kernel/head.S +0 -0
index 0a6e4f924df8..5e83e5b8a9de 100644
@@ -159,7 +159,7 @@ optional_header:
 

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
arch/arm64/kernel/head.S optional_header: · line 162
Quoted 4 lines
arch/arm64/kernel/head.S +1 -1
 extra_header_fields:
      .quad   0                               // ImageBase
-     .long   0x20                            // SectionAlignment
+     .long   0x1000                          // SectionAlignment
Looking at this again, I'm more confused than I was to begin with.

:-)

Quoted 5 lines

Surely we know exactly where the .text section will be loaded because of its VirtualAddress? If that were the case, we can drop the .align 12 as we already load it at the offset any arp or :lo12: immediate will have been built for.

No, not quite. It only tells us what the /offset/ should be of the section from the ImageBase chosen by the loader, not what the alignment of ImageBase itself should be. For instance, with a section VirtualAddress of 0x1000 and a SectionAlignment of 0x400, the section could legally be loaded @ 0x1400 or 0x1800 (for ImageBase == 0x400 or 0x800, respectively)

Signature · 1 line

Ard.

Quoted 34 lines

I don't follow how SectionAlignment interacts with a given section's VirtualAddress.
Thanks,
Mark.
Quoted 28 lines
Quoted 13 lines
arch/arm64/kernel/head.S section_table: · line 229
Quoted 9 lines
arch/arm64/kernel/head.S +1 -1
      .long   0x8                             // FileAlignment
      .short  0                               // MajorOperatingSystemVersion
      .short  0                               // MinorOperatingSystemVersion
@@ -226,7 +226,7 @@ section_table:
      .short  0               // NumberOfRelocations  (0 for executables)
      .short  0               // NumberOfLineNumbers  (0 for executables)
      .long   0xe0500020      // Characteristics (section flags)
-     .align 5
+     .align 12
Can we get a comment explaining why stext needs the additional alignment? Something like:
Quoted 9 lines
/*
* EFI will load stext onwards at the 4k section alignment
* described in the PE/COFF header. To ensure that instruction
* sequences using an adrp and a :lo12: immediate will function
* correctly at this alignment, we must ensure that stext is
* placed at a 4k boundary in the Image to begin with. */
.align 12
OK
--
Ard.

Mark Rutland

reviewer

2014-10-10 15:21 UTC #5 lore raw

PATCH 1/1 arm64/efi: set PE/COFF section alignment to 4 KB

Quoted 49 lines · Ard Biesheuvel

Quoted 32 lines · Mark Rutland
Quoted 31 lines · Ard Biesheuvel
Quoted 8 lines · Mark Rutland
Hi Ard,
Quoted 5 lines · Ard Biesheuvel
Position independent AArch64 code needs to be linked and loaded at the same relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will not work correctly. (This is how PC relative symbol references with a 4 GB reach are emitted)
Quoted 5 lines
Quoted 2 lines
We need to declare this in the PE/COFF header, otherwise the PE/COFF loader may load the Image and invoke the stub at an offset which violates this rule.
Has this been observed happening, or was this just found by inspection?
This is also something found by inspection, or rather, by the discussion going on in the other thread. I am not aware of any PE/COFF loaders that may choose an offset that is not 4 KB aligned, even if the header we give it appears to allow it.
Quoted 11 lines
Quoted 11 lines
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/head.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm64/kernel/head.S +0 -0
index 0a6e4f924df8..5e83e5b8a9de 100644
@@ -159,7 +159,7 @@ optional_header:
 
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
arch/arm64/kernel/head.S optional_header: · line 162
Quoted 4 lines
arch/arm64/kernel/head.S +1 -1
 extra_header_fields:
      .quad   0                               // ImageBase
-     .long   0x20                            // SectionAlignment
+     .long   0x1000                          // SectionAlignment
Looking at this again, I'm more confused than I was to begin with.
:-)
Quoted 5 lines
Surely we know exactly where the .text section will be loaded because of its VirtualAddress? If that were the case, we can drop the .align 12 as we already load it at the offset any arp or :lo12: immediate will have been built for.

Quoted 6 lines

No, not quite. It only tells us what the /offset/ should be of the section from the ImageBase chosen by the loader, not what the alignment of ImageBase itself should be. For instance, with a section VirtualAddress of 0x1000 and a SectionAlignment of 0x400, the section could legally be loaded @ 0x1400 or 0x1800 (for ImageBase == 0x400 or 0x800, respectively)

I see.

I had myself confused between the image and sections, and (mistakenly) thought we had control over the alignment of the image as opposed to sections. I thought the loader chose an ImageBase that was sufficiently aligned, then just loaded each segment at the requested offset from that. It sounds like the loader actually has to try to reconcile the offset of each section against each other to determine an ImageBase to use.

Given that the only thing we can control the alignment of is the .text section (with an offset applied below that), aligning the .text section to 4k sounds right.

Thanks for bearing with me!

Thanks,
Mark.

Roy Franz

2014-10-10 15:55 UTC #6 lore raw

PATCH 1/1 arm64/efi: set PE/COFF section alignment to 4 KB

In reply to Ard Biesheuvel

Quoted 12 lines · Ard Biesheuvel

Quoted 11 lines · Mark Rutland
Hi Ard,
Quoted 7 lines · Ard Biesheuvel
Position independent AArch64 code needs to be linked and loaded at the same relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will not work correctly. (This is how PC relative symbol references with a 4 GB reach are emitted)
We need to declare this in the PE/COFF header, otherwise the PE/COFF loader may load the Image and invoke the stub at an offset which violates this rule.

Quoted 8 lines

Quoted 2 lines
Has this been observed happening, or was this just found by inspection?
This is also something found by inspection, or rather, by the discussion going on in the other thread. I am not aware of any PE/COFF loaders that may choose an offset that is not 4 KB aligned, even if the header we give it appears to allow it.

This also resolves the problematic FileAlignment that I noticed when reviewing the branch to stext patch.

The PE/COFF spec description of FileAlignment

The alignment factor (in bytes) that is
used to align the raw data of sections
in the image file. The value should be
a power of 2 between 512 and 64 K,
inclusive. The default is 512. If the
SectionAlignment is less than the
architecture's page size, then
FileAlignment must match
SectionAlignment.

Previously the section alignment was less than the page size, and FileAlignment/SectionAlignment didn't match.

Reviewed-by Roy Franz <[email protected]>

Quoted 42 lines

Quoted 28 lines
Quoted 19 lines
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/head.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm64/kernel/head.S +1 -1
index 0a6e4f924df8..5e83e5b8a9de 100644
@@ -159,7 +159,7 @@ optional_header:
 
 extra_header_fields:
      .quad   0                               // ImageBase
-     .long   0x20                            // SectionAlignment
+     .long   0x1000                          // SectionAlignment
      .long   0x8                             // FileAlignment
      .short  0                               // MajorOperatingSystemVersion
      .short  0                               // MinorOperatingSystemVersion
arch/arm64/kernel/head.S section_table: · line 229
Quoted 5 lines
arch/arm64/kernel/head.S +1 -1
@@ -226,7 +226,7 @@ section_table:
      .short  0               // NumberOfRelocations  (0 for executables)
      .short  0               // NumberOfLineNumbers  (0 for executables)
      .long   0xe0500020      // Characteristics (section flags)
-     .align 5
+     .align 12
Can we get a comment explaining why stext needs the additional alignment? Something like:
Quoted 9 lines
/*
* EFI will load stext onwards at the 4k section alignment
* described in the PE/COFF header. To ensure that instruction
* sequences using an adrp and a :lo12: immediate will function
* correctly at this alignment, we must ensure that stext is
* placed at a 4k boundary in the Image to begin with. */
.align 12
OK
--
Ard.

Ard Biesheuvel

author

2014-10-10 15:59 UTC #7 lore raw

PATCH 1/1 arm64/efi: set PE/COFF section alignment to 4 KB

Quoted 38 lines · Roy Franz

Quoted 12 lines · Ard Biesheuvel
Quoted 11 lines · Mark Rutland
Hi Ard,
Quoted 7 lines · Ard Biesheuvel
Position independent AArch64 code needs to be linked and loaded at the same relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will not work correctly. (This is how PC relative symbol references with a 4 GB reach are emitted)
We need to declare this in the PE/COFF header, otherwise the PE/COFF loader may load the Image and invoke the stub at an offset which violates this rule.
Quoted 8 lines
Quoted 2 lines
Has this been observed happening, or was this just found by inspection?
This is also something found by inspection, or rather, by the discussion going on in the other thread. I am not aware of any PE/COFF loaders that may choose an offset that is not 4 KB aligned, even if the header we give it appears to allow it.
This also resolves the problematic FileAlignment that I noticed when reviewing the branch to stext patch.
The PE/COFF spec description of FileAlignment
The alignment factor (in bytes) that is
used to align the raw data of sections
in the image file. The value should be
a power of 2 between 512 and 64 K,
inclusive. The default is 512. If the
SectionAlignment is less than the
architecture's page size, then
FileAlignment must match
SectionAlignment.

Quoted 5 lines

Previously the section alignment was less than the page size, and FileAlignment/SectionAlignment didn't match.
Reviewed-by Roy Franz <[email protected]>

Yes, but this also means (as I noticed just now), that our file alignment is out of spec, as it should be at least 0x200. Unfortunately, that means we have to pad the kernel .data section to 0x200 alignment as well.

Oh, and our architecture does not have a single page size, so I wonder if we shouldn't just set both to 0x1000 and be done with it.

Thanks,
Ard.

Quoted 42 lines

Quoted 42 lines
Quoted 28 lines
Quoted 19 lines
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/kernel/head.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm64/kernel/head.S +1 -1
index 0a6e4f924df8..5e83e5b8a9de 100644
@@ -159,7 +159,7 @@ optional_header:
 
 extra_header_fields:
      .quad   0                               // ImageBase
-     .long   0x20                            // SectionAlignment
+     .long   0x1000                          // SectionAlignment
      .long   0x8                             // FileAlignment
      .short  0                               // MajorOperatingSystemVersion
      .short  0                               // MinorOperatingSystemVersion
arch/arm64/kernel/head.S section_table: · line 229
Quoted 5 lines
arch/arm64/kernel/head.S +1 -1
@@ -226,7 +226,7 @@ section_table:
      .short  0               // NumberOfRelocations  (0 for executables)
      .short  0               // NumberOfLineNumbers  (0 for executables)
      .long   0xe0500020      // Characteristics (section flags)
-     .align 5
+     .align 12
Can we get a comment explaining why stext needs the additional alignment? Something like:
Quoted 9 lines
/*
* EFI will load stext onwards at the 4k section alignment
* described in the PE/COFF header. To ensure that instruction
* sequences using an adrp and a :lo12: immediate will function
* correctly at this alignment, we must ensure that stext is
* placed at a 4k boundary in the Image to begin with. */
.align 12
OK
--
Ard.