Skip to contentSkip to conversation

ARM: efi: PE/COFF cleanup/hardening

PATCH New Ard Biesheuvel 2 participants 18 messages Jun 29, 2017 – Sep 8, 2017

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v2 cover ARM: efi: PE/COFF cleanup/hardening

This is the ARM counterpart of the changes now in v4.12 to clean up the PE/COFF header (which makes the kernel zImage loadable directly from UEFI), and to enhance it with hardening and debug features.

Russell: patches #4 - #7 need your ack before I can take them via the EFI tree. Please let me know if you have any objections, either to the patches themselves, or to them going via another tree. Thanks.

v1 blurb:

First of all, the cleanup consists of making the header comply with the PE/COFF spec (#1), removing the .reloc section (#2) and replacing all open coded constants with #defines from linux/pe.h (#3)

Patch #4 is a standalone patch that removes ksymtab/kcrctab sections that may get pulled in inadvertently when the decompressor is built with EFI support. Note that these sections are tiny and harmless by themselves, but the linker may dump them in unexpected places if they are not placed explicitly, which may interfere with the image layout. This is especially important when signing zImages for UEFI secure boot.

Patch #5 changes the description of the decompressor in memory, so that the UEFI firmware can apply strict ro/nx protections, resulting in a more secure execution environment for the UEFI stub.

Patch #6 splits the decompressor .start and .text output sections, so that the ELF view aligns with the PE/COFF view of the binary. This is necessary for patch #7 to work as expected.

Patch #7 enhances the decompressor binary with a NB10 Codeview debug entry referring to the path to arch/arm/boot/compressed/vmlinux on the build host. This is a debug feature that allows seamless source level single step debugging of the UEFI stub while executing in the context of the firmware.

v2: - rebase onto v4.12-rc7+
- simplify #3

Ard Biesheuvel (7):
arm: efi: remove forbidden values from the PE/COFF header
arm: efi: remove pointless dummy .reloc section
arm: efi: replace open coded constants with symbolic ones
arm: compressed: discard ksymtab/kcrctab sections
arm: efi: split zImage code and data into separate PE/COFF sections
arm: compressed: put zImage header and EFI header in dedicated section
arm: efi: add PE/COFF debug table to EFI header

 arch/arm/boot/compressed/Makefile      |   4 +
 arch/arm/boot/compressed/efi-header.S  | 214 ++++++++++++--------
 arch/arm/boot/compressed/vmlinux.lds.S |  39 +++-
 3 files changed, 168 insertions(+), 89 deletions(-)
Signature · 1 line
2.9.3
#1 lore raw

PATCH v2 1/7 arm: efi: remove forbidden values from the PE/COFF header

Bring the PE/COFF header in line with the PE/COFF spec, by setting NumberOfSymbols to 0, and removing the section alignment flags.

---
 arch/arm/boot/compressed/efi-header.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/arm/boot/compressed/efi-header.S +3 -3
index a17ca8d78656..4169b900d9d4 100644
@@ -44,7 +44,7 @@ coff_header:
 		.short	2			@ nr_sections
 		.long	0 			@ TimeDateStamp
 		.long	0			@ PointerToSymbolTable
-		.long	1			@ NumberOfSymbols
+		.long	0			@ NumberOfSymbols
 		.short	section_table - optional_header
 						@ SizeOfOptionalHeader
 		.short	0x306			@ Characteristics.
@@ -110,7 +110,7 @@ section_table:
 		.long	0			@ PointerToLineNumbers
 		.short	0			@ NumberOfRelocations
 		.short	0			@ NumberOfLineNumbers
-		.long	0x42100040		@ Characteristics
+		.long	0x42000040		@ Characteristics
 
 		.ascii	".text\0\0\0"
 		.long	_end - __efi_start	@ VirtualSize
@@ -121,7 +121,7 @@ section_table:
 		.long	0			@ PointerToLineNumbers
 		.short	0			@ NumberOfRelocations
 		.short	0			@ NumberOfLineNumbers
-		.long	0xe0500020		@ Characteristics
+		.long	0xe0000020		@ Characteristics
 
 		.align	9
 __efi_start:
Signature · 1 line
2.9.3
#2 lore raw

PATCH v2 2/7 arm: efi: remove pointless dummy .reloc section

In reply to Ard Biesheuvel

The kernel's EFI PE/COFF header contains a dummy .reloc section, and an explanatory comment that claims that this is required for the EFI application loader to accept the Image as a relocatable image (i.e., one that can be loaded at any offset and fixed up in place)

This was inherited from the x86 implementation, which has elaborate host tooling to mangle the PE/COFF header post-link time, and which populates the .reloc section with a single dummy base relocation. On ARM, no such tooling exists, and the .reloc section remains empty, and is never even exposed via the BaseRelocationTable directory entry, which is where the PE/COFF loader looks for it.

The PE/COFF spec is unclear about relocatable images that do not require any fixups, but the EDK2 implementation, which is the de facto reference for PE/COFF in the UEFI space, clearly does not care, and explicitly mentions (in a comment) that relocatable images with no base relocations are perfectly fine, as long as they don't have the RELOCS_STRIPPED attribute set (which is not the case for our PE/COFF image)

So simply remove the .reloc section altogether.

---
 arch/arm/boot/compressed/efi-header.S | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)
arch/arm/boot/compressed/efi-header.S +1 -17
index 4169b900d9d4..309d6198d335 100644
@@ -41,7 +41,7 @@ pe_header:
 
 coff_header:
 		.short	0x01c2			@ ARM or Thumb
-		.short	2			@ nr_sections
+		.short	1			@ nr_sections
 		.long	0 			@ TimeDateStamp
 		.long	0			@ PointerToSymbolTable
 		.long	0			@ NumberOfSymbols
@@ -96,22 +96,6 @@ extra_header_fields:
 		.quad	0			@ BaseRelocationTable
 
 section_table:
-		@
-		@ The EFI application loader requires a relocation section
-		@ because EFI applications must be relocatable. This is a
-		@ dummy section as far as we are concerned.
-		@
-		.ascii	".reloc\0\0"
-		.long	0			@ VirtualSize
-		.long	0			@ VirtualAddress
-		.long	0			@ SizeOfRawData
-		.long	0			@ PointerToRawData
-		.long	0			@ PointerToRelocations
-		.long	0			@ PointerToLineNumbers
-		.short	0			@ NumberOfRelocations
-		.short	0			@ NumberOfLineNumbers
-		.long	0x42000040		@ Characteristics
-
 		.ascii	".text\0\0\0"
 		.long	_end - __efi_start	@ VirtualSize
 		.long	__efi_start		@ VirtualAddress
Signature · 1 line
2.9.3
#3 lore raw

PATCH v2 3/7 arm: efi: replace open coded constants with symbolic ones

In reply to Ard Biesheuvel

Replace the various open coded constants in the EFI PE/COFF header with definitions from pe.h, or expressions based on local symbols.

---
 arch/arm/boot/compressed/efi-header.S | 128 ++++++++++----------
 1 file changed, 67 insertions(+), 61 deletions(-)
arch/arm/boot/compressed/efi-header.S +67 -61
index 309d6198d335..542e1ad432ae 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2015 Linaro Ltd
+ * Copyright (C) 2013-2017 Linaro Ltd
  * Authors: Roy Franz <[email protected]>
  *          Ard Biesheuvel <[email protected]>
  *
@@ -8,6 +8,9 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/pe.h>
+#include <linux/sizes.h>
+
 		.macro	__nop
 #ifdef CONFIG_EFI_STUB
 		@ This is almost but not quite a NOP, since it does clobber the
@@ -15,7 +18,7 @@
 		@ PE/COFF expects the magic string "MZ" at offset 0, while the
 		@ ARM/Linux boot protocol expects an executable instruction
 		@ there.
-		.inst	'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
+		.inst	MZ_MAGIC | (0x1310 << 16)	@ tstne r0, #0x4d000
 #else
  AR_CLASS(	mov	r0, r0		)
   M_CLASS(	nop.w			)
@@ -34,78 +37,81 @@
 		@ The only 2 fields of the MSDOS header that are used are this
 		@ PE/COFF offset, and the "MZ" bytes@offset 0x0.
 		@
-		.long	pe_header - start	@ Offset to the PE header.
+		.long	pe_header - start		@ Offset to the PE header.
 
 pe_header:
-		.ascii	"PE\0\0"
+		.long	PE_MAGIC
 
 coff_header:
-		.short	0x01c2			@ ARM or Thumb
-		.short	1			@ nr_sections
-		.long	0 			@ TimeDateStamp
-		.long	0			@ PointerToSymbolTable
-		.long	0			@ NumberOfSymbols
-		.short	section_table - optional_header
-						@ SizeOfOptionalHeader
-		.short	0x306			@ Characteristics.
-						@ IMAGE_FILE_32BIT_MACHINE |
-						@ IMAGE_FILE_DEBUG_STRIPPED |
-						@ IMAGE_FILE_EXECUTABLE_IMAGE |
-						@ IMAGE_FILE_LINE_NUMS_STRIPPED
+		.short	IMAGE_FILE_MACHINE_THUMB	@ Machine
+		.short	section_count			@ NumberOfSections
+		.long	0 				@ TimeDateStamp
+		.long	0				@ PointerToSymbolTable
+		.long	0				@ NumberOfSymbols
+		.short	section_table - optional_header	@ SizeOfOptionalHeader
+		.short	IMAGE_FILE_32BIT_MACHINE | \
+			IMAGE_FILE_DEBUG_STRIPPED | \
+			IMAGE_FILE_EXECUTABLE_IMAGE | \
+			IMAGE_FILE_LINE_NUMS_STRIPPED	@ Characteristics
 
 optional_header:
-		.short	0x10b			@ PE32 format
-		.byte	0x02			@ MajorLinkerVersion
-		.byte	0x14			@ MinorLinkerVersion
-		.long	_end - __efi_start	@ SizeOfCode
-		.long	0			@ SizeOfInitializedData
-		.long	0			@ SizeOfUninitializedData
-		.long	efi_stub_entry - start	@ AddressOfEntryPoint
-		.long	start_offset		@ BaseOfCode
-		.long	0			@ data
+		.short	PE_OPT_MAGIC_PE32		@ PE32 format
+		.byte	0x02				@ MajorLinkerVersion
+		.byte	0x14				@ MinorLinkerVersion
+		.long	_end - __efi_start		@ SizeOfCode
+		.long	0				@ SizeOfInitializedData
+		.long	0				@ SizeOfUninitializedData
+		.long	efi_stub_entry - start		@ AddressOfEntryPoint
+		.long	start_offset			@ BaseOfCode
+		.long	0				@ BaseOfData
 
 extra_header_fields:
-		.long	0			@ ImageBase
-		.long	0x200			@ SectionAlignment
-		.long	0x200			@ FileAlignment
-		.short	0			@ MajorOperatingSystemVersion
-		.short	0			@ MinorOperatingSystemVersion
-		.short	0			@ MajorImageVersion
-		.short	0			@ MinorImageVersion
-		.short	0			@ MajorSubsystemVersion
-		.short	0			@ MinorSubsystemVersion
-		.long	0			@ Win32VersionValue
+		.long	0				@ ImageBase
+		.long	SZ_512				@ SectionAlignment
+		.long	SZ_512				@ FileAlignment
+		.short	0				@ MajorOsVersion
+		.short	0				@ MinorOsVersion
+		.short	0				@ MajorImageVersion
+		.short	0				@ MinorImageVersion
+		.short	0				@ MajorSubsystemVersion
+		.short	0				@ MinorSubsystemVersion
+		.long	0				@ Win32VersionValue
 
-		.long	_end - start		@ SizeOfImage
-		.long	start_offset		@ SizeOfHeaders
-		.long	0			@ CheckSum
-		.short	0xa			@ Subsystem (EFI application)
-		.short	0			@ DllCharacteristics
-		.long	0			@ SizeOfStackReserve
-		.long	0			@ SizeOfStackCommit
-		.long	0			@ SizeOfHeapReserve
-		.long	0			@ SizeOfHeapCommit
-		.long	0			@ LoaderFlags
-		.long	0x6			@ NumberOfRvaAndSizes
+		.long	_end - start			@ SizeOfImage
+		.long	start_offset			@ SizeOfHeaders
+		.long	0				@ CheckSum
+		.short	IMAGE_SUBSYSTEM_EFI_APPLICATION	@ Subsystem
+		.short	0				@ DllCharacteristics
+		.long	0				@ SizeOfStackReserve
+		.long	0				@ SizeOfStackCommit
+		.long	0				@ SizeOfHeapReserve
+		.long	0				@ SizeOfHeapCommit
+		.long	0				@ LoaderFlags
+		.long	(section_table - .) / 8		@ NumberOfRvaAndSizes
 
-		.quad	0			@ ExportTable
-		.quad	0			@ ImportTable
-		.quad	0			@ ResourceTable
-		.quad	0			@ ExceptionTable
-		.quad	0			@ CertificationTable
-		.quad	0			@ BaseRelocationTable
+		.quad	0				@ ExportTable
+		.quad	0				@ ImportTable
+		.quad	0				@ ResourceTable
+		.quad	0				@ ExceptionTable
+		.quad	0				@ CertificationTable
+		.quad	0				@ BaseRelocationTable
 
 section_table:
 		.ascii	".text\0\0\0"
-		.long	_end - __efi_start	@ VirtualSize
-		.long	__efi_start		@ VirtualAddress
-		.long	_edata - __efi_start	@ SizeOfRawData
-		.long	__efi_start		@ PointerToRawData
-		.long	0			@ PointerToRelocations
-		.long	0			@ PointerToLineNumbers
-		.short	0			@ NumberOfRelocations
-		.short	0			@ NumberOfLineNumbers
-		.long	0xe0000020		@ Characteristics
+		.long	_end - __efi_start		@ VirtualSize
+		.long	__efi_start			@ VirtualAddress
+		.long	_edata - __efi_start		@ SizeOfRawData
+		.long	__efi_start			@ PointerToRawData
+		.long	0				@ PointerToRelocations
+		.long	0				@ PointerToLineNumbers
+		.short	0				@ NumberOfRelocations
+		.short	0				@ NumberOfLineNumbers
+		.long	IMAGE_SCN_CNT_CODE | \
+			IMAGE_SCN_MEM_READ | \
+			IMAGE_SCN_MEM_WRITE | \
+			IMAGE_SCN_MEM_EXECUTE		@ Characteristics
+
+		.set	section_count, (. - section_table) / 40
 
 		.align	9
 __efi_start:
Signature · 1 line
2.9.3
#4 lore raw

PATCH v2 4/7 arm: compressed: discard ksymtab/kcrctab sections

In reply to Ard Biesheuvel

With UEFI support added to the decompressor, we may end up including C code that is part of the kernel proper, but is rebuilt for the decompressor. This may result in ksymtab/kcrctab metadata being duplicated into the decompressor, so discard such regions explicitly.

---
 arch/arm/boot/compressed/vmlinux.lds.S | 5 +++++
 1 file changed, 5 insertions(+)
arch/arm/boot/compressed/vmlinux.lds.S +5 -0
index 81c493156ce8..1fa62432e283 100644
@@ -29,6 +29,11 @@ SECTIONS
      * of the text/got segments.
      */
     *(.data)
+    /*
+     * C code that is shared with the kernel proper (but rebuilt for the
+     * decompressor) may contain exports that we have no use for here.
+     */
+    *(*ksymtab* *kcrctab*)
   }
 
   . = TEXT_START;
Signature · 1 line
2.9.3
#5 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

In reply to Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

---
 arch/arm/boot/compressed/efi-header.S  | 32 ++++++++++++++------
 arch/arm/boot/compressed/vmlinux.lds.S | 30 +++++++++++++-----
 2 files changed, 46 insertions(+), 16 deletions(-)
arch/arm/boot/compressed/efi-header.S +23 -9
index 542e1ad432ae..c94a88ae834d 100644
@@ -54,20 +54,22 @@ coff_header:
 			IMAGE_FILE_EXECUTABLE_IMAGE | \
 			IMAGE_FILE_LINE_NUMS_STRIPPED	@ Characteristics
 
+#define __pecoff_code_size (__pecoff_data_start - __efi_start)
+
 optional_header:
 		.short	PE_OPT_MAGIC_PE32		@ PE32 format
 		.byte	0x02				@ MajorLinkerVersion
 		.byte	0x14				@ MinorLinkerVersion
-		.long	_end - __efi_start		@ SizeOfCode
-		.long	0				@ SizeOfInitializedData
+		.long	__pecoff_code_size		@ SizeOfCode
+		.long	__pecoff_data_size		@ SizeOfInitializedData
 		.long	0				@ SizeOfUninitializedData
 		.long	efi_stub_entry - start		@ AddressOfEntryPoint
 		.long	start_offset			@ BaseOfCode
-		.long	0				@ BaseOfData
+		.long	__pecoff_data_start - start	@ BaseOfData
 
 extra_header_fields:
 		.long	0				@ ImageBase
-		.long	SZ_512				@ SectionAlignment
+		.long	SZ_4K				@ SectionAlignment
 		.long	SZ_512				@ FileAlignment
 		.short	0				@ MajorOsVersion
 		.short	0				@ MinorOsVersion
@@ -77,7 +79,7 @@ extra_header_fields:
 		.short	0				@ MinorSubsystemVersion
 		.long	0				@ Win32VersionValue
 
-		.long	_end - start			@ SizeOfImage
+		.long	__pecoff_end - start		@ SizeOfImage
 		.long	start_offset			@ SizeOfHeaders
 		.long	0				@ CheckSum
 		.short	IMAGE_SUBSYSTEM_EFI_APPLICATION	@ Subsystem
@@ -98,9 +100,9 @@ extra_header_fields:
 
 section_table:
 		.ascii	".text\0\0\0"
-		.long	_end - __efi_start		@ VirtualSize
+		.long	__pecoff_code_size		@ VirtualSize
 		.long	__efi_start			@ VirtualAddress
-		.long	_edata - __efi_start		@ SizeOfRawData
+		.long	__pecoff_code_size		@ SizeOfRawData
 		.long	__efi_start			@ PointerToRawData
 		.long	0				@ PointerToRelocations
 		.long	0				@ PointerToLineNumbers
@@ -108,12 +110,24 @@ section_table:
 		.short	0				@ NumberOfLineNumbers
 		.long	IMAGE_SCN_CNT_CODE | \
 			IMAGE_SCN_MEM_READ | \
-			IMAGE_SCN_MEM_WRITE | \
 			IMAGE_SCN_MEM_EXECUTE		@ Characteristics
 
+		.ascii	".data\0\0\0"
+		.long	__pecoff_data_size		@ VirtualSize
+		.long	__pecoff_data_start - start	@ VirtualAddress
+		.long	__pecoff_data_rawsize		@ SizeOfRawData
+		.long	__pecoff_data_start - start	@ PointerToRawData
+		.long	0				@ PointerToRelocations
+		.long	0				@ PointerToLineNumbers
+		.short	0				@ NumberOfRelocations
+		.short	0				@ NumberOfLineNumbers
+		.long	IMAGE_SCN_CNT_INITIALIZED_DATA | \
+			IMAGE_SCN_MEM_READ | \
+			IMAGE_SCN_MEM_WRITE		@ Characteristics
+
 		.set	section_count, (. - section_table) / 40
 
-		.align	9
+		.align	12
 __efi_start:
 #endif
 		.endm
arch/arm/boot/compressed/vmlinux.lds.S +23 -7
index 1fa62432e283..dfcc2baa0077 100644
@@ -53,13 +53,6 @@ SECTIONS
     *(.rodata)
     *(.rodata.*)
   }
-  .data : {
-    /*
-     * The EFI stub always executes from RAM, and runs strictly before the
-     * decompressor, so we can make an exception for its r/w data, and keep it
-     */
-    *(.data.efistub)
-  }
   .piggydata : {
     *(.piggydata)
   }
@@ -75,6 +68,26 @@ SECTIONS
   /* ensure the zImage file size is always a multiple of 64 bits */
   /* (without a dummy byte, ld just ignores the empty section) */
   .pad			: { BYTE(0); . = ALIGN(8); }
+
+#ifdef CONFIG_EFI_STUB
+  .data : ALIGN(4096) {
+    __pecoff_data_start = .;
+    /*
+     * The EFI stub always executes from RAM, and runs strictly before the
+     * decompressor, so we can make an exception for its r/w data, and keep it
+     */
+    *(.data.efistub)
+    __pecoff_data_end = .;
+
+    /*
+     * PE/COFF mandates a file size which is a multiple of 512 bytes if the
+     * section size equals or exceeds 4 KB
+     */
+    . = ALIGN(512);
+  }
+  __pecoff_data_rawsize = . - ADDR(.data);
+#endif
+
   _edata = .;
 
   _magic_sig = ZIMAGE_MAGIC(0x016f2818);
@@ -89,6 +102,9 @@ SECTIONS
   . = ALIGN(8);		/* the stack must be 64-bit aligned */
   .stack		: { *(.stack) }
 
+  PROVIDE(__pecoff_data_size = ALIGN(512) - ADDR(.data));
+  PROVIDE(__pecoff_end = ALIGN(512));
+
   .stab 0		: { *(.stab) }
   .stabstr 0		: { *(.stabstr) }
   .stab.excl 0		: { *(.stab.excl) }
Signature · 1 line
2.9.3
#6 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Currently the solution is to revert this patch.

Have you a better option?

Thanks,

Gregory

Quoted 150 lines
---
 arch/arm/boot/compressed/efi-header.S  | 32 ++++++++++++++------
 arch/arm/boot/compressed/vmlinux.lds.S | 30 +++++++++++++-----
 2 files changed, 46 insertions(+), 16 deletions(-)
arch/arm/boot/compressed/efi-header.S +23 -9
index 542e1ad432ae..c94a88ae834d 100644
@@ -54,20 +54,22 @@ coff_header:
 			IMAGE_FILE_EXECUTABLE_IMAGE | \
 			IMAGE_FILE_LINE_NUMS_STRIPPED	@ Characteristics
 
+#define __pecoff_code_size (__pecoff_data_start - __efi_start)
+
 optional_header:
 		.short	PE_OPT_MAGIC_PE32		@ PE32 format
 		.byte	0x02				@ MajorLinkerVersion
 		.byte	0x14				@ MinorLinkerVersion
-		.long	_end - __efi_start		@ SizeOfCode
-		.long	0				@ SizeOfInitializedData
+		.long	__pecoff_code_size		@ SizeOfCode
+		.long	__pecoff_data_size		@ SizeOfInitializedData
 		.long	0				@ SizeOfUninitializedData
 		.long	efi_stub_entry - start		@ AddressOfEntryPoint
 		.long	start_offset			@ BaseOfCode
-		.long	0				@ BaseOfData
+		.long	__pecoff_data_start - start	@ BaseOfData
 
 extra_header_fields:
 		.long	0				@ ImageBase
-		.long	SZ_512				@ SectionAlignment
+		.long	SZ_4K				@ SectionAlignment
 		.long	SZ_512				@ FileAlignment
 		.short	0				@ MajorOsVersion
 		.short	0				@ MinorOsVersion
@@ -77,7 +79,7 @@ extra_header_fields:
 		.short	0				@ MinorSubsystemVersion
 		.long	0				@ Win32VersionValue
 
-		.long	_end - start			@ SizeOfImage
+		.long	__pecoff_end - start		@ SizeOfImage
 		.long	start_offset			@ SizeOfHeaders
 		.long	0				@ CheckSum
 		.short	IMAGE_SUBSYSTEM_EFI_APPLICATION	@ Subsystem
@@ -98,9 +100,9 @@ extra_header_fields:
 
 section_table:
 		.ascii	".text\0\0\0"
-		.long	_end - __efi_start		@ VirtualSize
+		.long	__pecoff_code_size		@ VirtualSize
 		.long	__efi_start			@ VirtualAddress
-		.long	_edata - __efi_start		@ SizeOfRawData
+		.long	__pecoff_code_size		@ SizeOfRawData
 		.long	__efi_start			@ PointerToRawData
 		.long	0				@ PointerToRelocations
 		.long	0				@ PointerToLineNumbers
@@ -108,12 +110,24 @@ section_table:
 		.short	0				@ NumberOfLineNumbers
 		.long	IMAGE_SCN_CNT_CODE | \
 			IMAGE_SCN_MEM_READ | \
-			IMAGE_SCN_MEM_WRITE | \
 			IMAGE_SCN_MEM_EXECUTE		@ Characteristics
 
+		.ascii	".data\0\0\0"
+		.long	__pecoff_data_size		@ VirtualSize
+		.long	__pecoff_data_start - start	@ VirtualAddress
+		.long	__pecoff_data_rawsize		@ SizeOfRawData
+		.long	__pecoff_data_start - start	@ PointerToRawData
+		.long	0				@ PointerToRelocations
+		.long	0				@ PointerToLineNumbers
+		.short	0				@ NumberOfRelocations
+		.short	0				@ NumberOfLineNumbers
+		.long	IMAGE_SCN_CNT_INITIALIZED_DATA | \
+			IMAGE_SCN_MEM_READ | \
+			IMAGE_SCN_MEM_WRITE		@ Characteristics
+
 		.set	section_count, (. - section_table) / 40
 
-		.align	9
+		.align	12
 __efi_start:
 #endif
 		.endm
arch/arm/boot/compressed/vmlinux.lds.S +23 -7
index 1fa62432e283..dfcc2baa0077 100644
@@ -53,13 +53,6 @@ SECTIONS
     *(.rodata)
     *(.rodata.*)
   }
-  .data : {
-    /*
-     * The EFI stub always executes from RAM, and runs strictly before the
-     * decompressor, so we can make an exception for its r/w data, and keep it
-     */
-    *(.data.efistub)
-  }
   .piggydata : {
     *(.piggydata)
   }
@@ -75,6 +68,26 @@ SECTIONS
   /* ensure the zImage file size is always a multiple of 64 bits */
   /* (without a dummy byte, ld just ignores the empty section) */
   .pad			: { BYTE(0); . = ALIGN(8); }
+
+#ifdef CONFIG_EFI_STUB
+  .data : ALIGN(4096) {
+    __pecoff_data_start = .;
+    /*
+     * The EFI stub always executes from RAM, and runs strictly before the
+     * decompressor, so we can make an exception for its r/w data, and keep it
+     */
+    *(.data.efistub)
+    __pecoff_data_end = .;
+
+    /*
+     * PE/COFF mandates a file size which is a multiple of 512 bytes if the
+     * section size equals or exceeds 4 KB
+     */
+    . = ALIGN(512);
+  }
+  __pecoff_data_rawsize = . - ADDR(.data);
+#endif
+
   _edata = .;
 
   _magic_sig = ZIMAGE_MAGIC(0x016f2818);
@@ -89,6 +102,9 @@ SECTIONS
   . = ALIGN(8);		/* the stack must be 64-bit aligned */
   .stack		: { *(.stack) }
 
+  PROVIDE(__pecoff_data_size = ALIGN(512) - ADDR(.data));
+  PROVIDE(__pecoff_end = ALIGN(512));
+
   .stab 0		: { *(.stab) }
   .stabstr 0		: { *(.stabstr) }
   .stab.excl 0		: { *(.stab.excl) }
Signature · 7 lines
2.9.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#7 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Quoted 16 lines · Gregory CLEMENT

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

Quoted 6 lines

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

Ouch.

Quoted 10 lines

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Currently the solution is to revert this patch.

Have you a better option?

I will investigate.

#8 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Quoted 33 lines · Ard Biesheuvel
Quoted 16 lines · Gregory CLEMENT

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

Quoted 6 lines

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

Ouch.

Quoted 6 lines

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Quoted 6 lines
Quoted 4 lines

Currently the solution is to revert this patch.

Have you a better option?

I will investigate.

I cannot reproduce this on QEMU or my Beaglebone white. I have tried a locally built zImage as well as the one built by kernelci.

Could you please try whether this fixes things? It does not explain anything but it will help me figure out what is going on (hopefully)

(quoted diff) +0 -0

b/arch/arm/boot/compressed/efi-header.S
index c94a88ae834d..671a6e5b7b99 100644

arch/arm/boot/compressed/efi-header.S +1 -1
@@ -127,7 +127,7 @@ section_table:
 
                .set    section_count, (. - section_table) / 40
 
-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm
#9 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Hi Ard,

Quoted 39 lines · Ard Biesheuvel
Quoted 38 lines · Ard Biesheuvel
Quoted 16 lines · Gregory CLEMENT

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

Quoted 6 lines

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

Ouch.

Quoted 10 lines

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Currently the solution is to revert this patch.

Have you a better option?

Quoted 7 lines
Quoted 1 line

I will investigate.

I cannot reproduce this on QEMU or my Beaglebone white. I have tried a locally built zImage as well as the one built by kernelci.

Could you please try whether this fixes things? It does not explain anything but it will help me figure out what is going on (hopefully)

I've just tested this change and it didn't fix anything.

Gregory

Quoted 16 lines
arch/arm/boot/compressed/efi-header.S +1 -1
index c94a88ae834d..671a6e5b7b99 100644
@@ -127,7 +127,7 @@ section_table:
 
                .set    section_count, (. - section_table) / 40
 
-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#10 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Quoted 64 lines · Gregory CLEMENT

Hi Ard,

Quoted 39 lines · Ard Biesheuvel
Quoted 38 lines · Ard Biesheuvel
Quoted 16 lines · Gregory CLEMENT

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

Quoted 6 lines

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

Ouch.

Quoted 10 lines

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Currently the solution is to revert this patch.

Have you a better option?

Quoted 7 lines
Quoted 1 line

I will investigate.

I cannot reproduce this on QEMU or my Beaglebone white. I have tried a locally built zImage as well as the one built by kernelci.

Could you please try whether this fixes things? It does not explain anything but it will help me figure out what is going on (hopefully)

I've just tested this change and it didn't fix anything.

Gregory

Quoted 9 lines
arch/arm/boot/compressed/efi-header.S +0 -0
index c94a88ae834d..671a6e5b7b99 100644
@@ -127,7 +127,7 @@ section_table:
 
Quoted 8 lines

arch/arm/boot/compressed/efi-header.S section_table: · line 133

Quoted 7 lines
arch/arm/boot/compressed/efi-header.S +1 -1
                .set    section_count, (. - section_table) / 40
 
-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm

How about this?

(quoted diff) +0 -0

b/arch/arm/boot/compressed/vmlinux.lds.S
index 7a4c59154361..dfcc2baa0077 100644

arch/arm/boot/compressed/vmlinux.lds.S +5 -0
@@ -29,6 +29,11 @@ SECTIONS
      * of the text/got segments.
      */
     *(.data)
+    /*
+     * C code that is shared with the kernel proper (but rebuilt for the
+     * decompressor) may contain exports that we have no use for here.
+     */
+    *(*ksymtab* *kcrctab*)
   }
 
   . = TEXT_START;
#11 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Hi Ard,

Quoted 76 lines · Ard Biesheuvel
Quoted 64 lines · Gregory CLEMENT

Hi Ard,

Quoted 39 lines · Ard Biesheuvel
Quoted 38 lines · Ard Biesheuvel
Quoted 16 lines · Gregory CLEMENT

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

Quoted 6 lines

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

Ouch.

Quoted 10 lines

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Currently the solution is to revert this patch.

Have you a better option?

Quoted 7 lines
Quoted 1 line

I will investigate.

I cannot reproduce this on QEMU or my Beaglebone white. I have tried a locally built zImage as well as the one built by kernelci.

Could you please try whether this fixes things? It does not explain anything but it will help me figure out what is going on (hopefully)

I've just tested this change and it didn't fix anything.

Gregory

Quoted 9 lines
arch/arm/boot/compressed/efi-header.S +0 -0
index c94a88ae834d..671a6e5b7b99 100644
@@ -127,7 +127,7 @@ section_table:
 
Quoted 8 lines

arch/arm/boot/compressed/efi-header.S section_table: · line 133

Quoted 7 lines
arch/arm/boot/compressed/efi-header.S +1 -1
                .set    section_count, (. - section_table) / 40
 
-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm
Quoted 1 line

How about this?

It fixed the bug! (I tested with and without your previous patch and it worked in both case)

When you will send your patch, you can add my:

Thanks,

Gregory

Quoted 23 lines
arch/arm/boot/compressed/vmlinux.lds.S +5 -0
index 7a4c59154361..dfcc2baa0077 100644
@@ -29,6 +29,11 @@ SECTIONS
      * of the text/got segments.
      */
     *(.data)
+    /*
+     * C code that is shared with the kernel proper (but rebuilt for the
+     * decompressor) may contain exports that we have no use for here.
+     */
+    *(*ksymtab* *kcrctab*)
   }
 
   . = TEXT_START;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#12 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Quoted 85 lines · Gregory CLEMENT

Hi Ard,

Quoted 76 lines · Ard Biesheuvel
Quoted 64 lines · Gregory CLEMENT

Hi Ard,

Quoted 39 lines · Ard Biesheuvel
Quoted 38 lines · Ard Biesheuvel
Quoted 16 lines · Gregory CLEMENT

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

Quoted 6 lines

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

Ouch.

Quoted 10 lines

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Currently the solution is to revert this patch.

Have you a better option?

Quoted 7 lines
Quoted 1 line

I will investigate.

I cannot reproduce this on QEMU or my Beaglebone white. I have tried a locally built zImage as well as the one built by kernelci.

Could you please try whether this fixes things? It does not explain anything but it will help me figure out what is going on (hopefully)

I've just tested this change and it didn't fix anything.

Gregory

Quoted 9 lines
arch/arm/boot/compressed/efi-header.S +0 -0
index c94a88ae834d..671a6e5b7b99 100644
@@ -127,7 +127,7 @@ section_table:
 
Quoted 8 lines

arch/arm/boot/compressed/efi-header.S section_table: · line 133

Quoted 7 lines
arch/arm/boot/compressed/efi-header.S +1 -1
                .set    section_count, (. - section_table) / 40
 
-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm
Quoted 1 line

How about this?

It fixed the bug! (I tested with and without your previous patch and it worked in both case)

Quoted 7 lines

When you will send your patch, you can add my:

Thanks,

Gregory

Thanks a lot. I will send it out today.

#13 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Quoted 82 lines · Ard Biesheuvel
Quoted 80 lines · Gregory CLEMENT

Hi Ard,

Quoted 76 lines · Ard Biesheuvel
Quoted 72 lines · Gregory CLEMENT

Hi Ard,

Quoted 39 lines · Ard Biesheuvel
Quoted 38 lines · Ard Biesheuvel
Quoted 16 lines · Gregory CLEMENT

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

Quoted 6 lines

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

Ouch.

Quoted 10 lines

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Currently the solution is to revert this patch.

Have you a better option?

Quoted 7 lines
Quoted 1 line

I will investigate.

I cannot reproduce this on QEMU or my Beaglebone white. I have tried a locally built zImage as well as the one built by kernelci.

Could you please try whether this fixes things? It does not explain anything but it will help me figure out what is going on (hopefully)

I've just tested this change and it didn't fix anything.

Gregory

Quoted 16 lines
arch/arm/boot/compressed/efi-header.S +1 -1
index c94a88ae834d..671a6e5b7b99 100644
@@ -127,7 +127,7 @@ section_table:
 
                .set    section_count, (. - section_table) / 40
 
-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm
Quoted 8 lines
Quoted 8 lines
Quoted 1 line

How about this?

It fixed the bug! (I tested with and without your previous patch and it worked in both case)

When you will send your patch, you can add my:

Would you mind checking whether this fixes the issue as well?

arch/arm/boot/compressed/piggy.S +1 -0
index f72088495f43..5d52c556dd32 100644
@@ -1,5 +1,6 @@
        .section .piggydata,#alloc
        .globl  input_data
+       .align  2
 input_data:
        .incbin "arch/arm/boot/compressed/piggy_data"
        .globl  input_data_end

There may be other reasons than ksymtab entries that could result in piggydata ending up unaligned in the decompressor (which is what caused the issue before)
This is a better fix, because it addresses the root cause.

#14 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Hi Ard,

Quoted 106 lines · Ard Biesheuvel
Quoted 82 lines · Ard Biesheuvel
Quoted 80 lines · Gregory CLEMENT

Hi Ard,

Quoted 76 lines · Ard Biesheuvel
Quoted 72 lines · Gregory CLEMENT

Hi Ard,

Quoted 39 lines · Ard Biesheuvel
Quoted 38 lines · Ard Biesheuvel
Quoted 16 lines · Gregory CLEMENT

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

Quoted 6 lines

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

Ouch.

Quoted 10 lines

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Currently the solution is to revert this patch.

Have you a better option?

Quoted 7 lines
Quoted 1 line

I will investigate.

I cannot reproduce this on QEMU or my Beaglebone white. I have tried a locally built zImage as well as the one built by kernelci.

Could you please try whether this fixes things? It does not explain anything but it will help me figure out what is going on (hopefully)

I've just tested this change and it didn't fix anything.

Gregory

Quoted 16 lines
arch/arm/boot/compressed/efi-header.S +1 -1
index c94a88ae834d..671a6e5b7b99 100644
@@ -127,7 +127,7 @@ section_table:
 
                .set    section_count, (. - section_table) / 40
 
-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm
Quoted 8 lines
Quoted 8 lines
Quoted 1 line

How about this?

It fixed the bug! (I tested with and without your previous patch and it worked in both case)

When you will send your patch, you can add my:

Would you mind checking whether this fixes the issue as well?

arch/arm/boot/compressed/piggy.S +1 -0
index f72088495f43..5d52c556dd32 100644
@@ -1,5 +1,6 @@
        .section .piggydata,#alloc
        .globl  input_data
+       .align  2
 input_data:
        .incbin "arch/arm/boot/compressed/piggy_data"
        .globl  input_data_end
Quoted 4 lines

There may be other reasons than ksymtab entries that could result in piggydata ending up unaligned in the decompressor (which is what caused the issue before)
This is a better fix, because it addresses the root cause.

I've tested this single patch and it does not fix the boot issue :(

Gregory

Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#15 lore raw

PATCH v2 5/7 arm: efi: split zImage code and data into separate PE/COFF sections

Quoted 110 lines · Gregory CLEMENT

Hi Ard,

Quoted 106 lines · Ard Biesheuvel
Quoted 82 lines · Ard Biesheuvel
Quoted 80 lines · Gregory CLEMENT

Hi Ard,

Quoted 76 lines · Ard Biesheuvel
Quoted 72 lines · Gregory CLEMENT

Hi Ard,

Quoted 39 lines · Ard Biesheuvel
Quoted 38 lines · Ard Biesheuvel
Quoted 16 lines · Gregory CLEMENT

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

To prevent unintended modifications to the kernel text (malicious or otherwise) while running the EFI stub, describe the kernel image as two separate sections: a .text section with read-execute permissions, covering .text, .rodata, .piggytext and the GOT sections (which the stub does not care about anyway), and a .data section with read-write permissions, covering .data and .bss.

This relies on the firmware to actually take the section permission flags into account, but this is something that is currently being implemented in EDK2, which means we will likely start seeing it in the wild between one and two years from now.

Quoted 6 lines

This patch had been merged in mainline yesterday and now prevent the Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect that more Socs are impacted because the number of boot fail exploded according to kci:
https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/

Ouch.

Quoted 10 lines

I found this patch after bisecting (I can provide the bisect log if needed).

The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in multi_v7_defconfig but not with mvebu_v7_defconfig.

Currently the solution is to revert this patch.

Have you a better option?

Quoted 7 lines
Quoted 1 line

I will investigate.

I cannot reproduce this on QEMU or my Beaglebone white. I have tried a locally built zImage as well as the one built by kernelci.

Could you please try whether this fixes things? It does not explain anything but it will help me figure out what is going on (hopefully)

I've just tested this change and it didn't fix anything.

Gregory

Quoted 16 lines
arch/arm/boot/compressed/efi-header.S +1 -1
index c94a88ae834d..671a6e5b7b99 100644
@@ -127,7 +127,7 @@ section_table:
 
                .set    section_count, (. - section_table) / 40
 
-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm
Quoted 8 lines
Quoted 8 lines
Quoted 1 line

How about this?

It fixed the bug! (I tested with and without your previous patch and it worked in both case)

When you will send your patch, you can add my:

Would you mind checking whether this fixes the issue as well?

arch/arm/boot/compressed/piggy.S +1 -0
index f72088495f43..5d52c556dd32 100644
@@ -1,5 +1,6 @@
        .section .piggydata,#alloc
        .globl  input_data
+       .align  2
 input_data:
        .incbin "arch/arm/boot/compressed/piggy_data"
        .globl  input_data_end
Quoted 7 lines
Quoted 4 lines

There may be other reasons than ksymtab entries that could result in piggydata ending up unaligned in the decompressor (which is what caused the issue before)
This is a better fix, because it addresses the root cause.

I've tested this single patch and it does not fix the boot issue :(

OK, strange. Anyway, thanks for testing. I will post the original fix then.

#16 lore raw

PATCH v2 6/7 arm: compressed: put zImage header and EFI header in dedicated section

In reply to Ard Biesheuvel

To align the PE/COFF and the ELF headers of the decompressor binary, put the zImage header and the EFI header in a separate .start section, and emit it at the start of the Image. This change is necessary for UEFI based debug tooling to be able to use the vmlinux ELF binary, since it gets confused if the PE/COFF .text section and the ELF .text section live at different offsets.

---
 arch/arm/boot/compressed/efi-header.S  | 1 +
 arch/arm/boot/compressed/vmlinux.lds.S | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)
arch/arm/boot/compressed/efi-header.S +1 -0
index c94a88ae834d..a14516f7a27e 100644
@@ -129,5 +129,6 @@ section_table:
 
 		.align	12
 __efi_start:
+		.text
 #endif
 		.endm
arch/arm/boot/compressed/vmlinux.lds.S +3 -1
index dfcc2baa0077..aa0ee9b5affe 100644
@@ -39,9 +39,11 @@ SECTIONS
   . = TEXT_START;
   _text = .;
 
-  .text : {
+  .start : {
     _start = .;
     *(.start)
+  }
+  .text : {
     *(.text)
     *(.text.*)
     *(.fixup)
Signature · 1 line
2.9.3
#17 lore raw

PATCH v2 7/7 arm: efi: add PE/COFF debug table to EFI header

In reply to Ard Biesheuvel

This updates the PE/COFF header to emit the absolute path to the decompressor vmlinux ELF file into a so-called NB10 Codeview entry. This is hugely helpful when debugging the firmware->stub handover.

---
 arch/arm/boot/compressed/Makefile     |  4 ++
 arch/arm/boot/compressed/efi-header.S | 47 ++++++++++++++++++++
 2 files changed, 51 insertions(+)
arch/arm/boot/compressed/Makefile +4 -0
index d50430c40045..6b978bdbac3e 100644
@@ -196,3 +196,7 @@ AFLAGS_hyp-stub.o := -Wa,-march=armv7-a
 
 $(obj)/hyp-stub.S: $(srctree)/arch/$(SRCARCH)/kernel/hyp-stub.S
 	$(call cmd,shipped)
+
+ifeq ($(CONFIG_EFI)$(CONFIG_DEBUG_INFO),yy)
+AFLAGS_head.o += -DVMLINUX_PATH="\"$(realpath $(obj)/vmlinux)\""
+endif
arch/arm/boot/compressed/efi-header.S +47 -0

59 lines not on this page · raw message

Signature · 1 line
2.9.3