[PATCH v2 0/7] ARM: efi: PE/COFF cleanup/hardening

[PATCH v2 0/7] ARM: efi: PE/COFF cleanup/hardening

[Ard Biesheuvel]

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(-)

-- 
2.9.3

[PATCH v2 0/7] ARM: efi: PE/COFF cleanup/hardening

[Ard Biesheuvel]

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(-)

-- 
2.9.3

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

[Ard Biesheuvel]

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/compressed/efi-header.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index a17ca8d78656..4169b900d9d4 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -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:
-- 
2.9.3

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

[Ard Biesheuvel]

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/boot/compressed/efi-header.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index a17ca8d78656..4169b900d9d4 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -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:
-- 
2.9.3

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

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/compressed/efi-header.S | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index 4169b900d9d4..309d6198d335 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -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
-- 
2.9.3

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

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/boot/compressed/efi-header.S | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index 4169b900d9d4..309d6198d335 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -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
-- 
2.9.3

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

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/compressed/efi-header.S | 128 ++++++++++----------
 1 file changed, 67 insertions(+), 61 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index 309d6198d335..542e1ad432ae 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2015 Linaro Ltd
+ * Copyright (C) 2013-2017 Linaro Ltd
  * Authors: Roy Franz <roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  *          Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  *
@@ -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 at 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:
-- 
2.9.3

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

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/boot/compressed/efi-header.S | 128 ++++++++++----------
 1 file changed, 67 insertions(+), 61 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index 309d6198d335..542e1ad432ae 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2015 Linaro Ltd
+ * Copyright (C) 2013-2017 Linaro Ltd
  * Authors: Roy Franz <roy.franz@linaro.org>
  *          Ard Biesheuvel <ard.biesheuvel@linaro.org>
  *
@@ -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:
-- 
2.9.3

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

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

Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/compressed/vmlinux.lds.S | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index 81c493156ce8..1fa62432e283 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -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;
-- 
2.9.3

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

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

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/boot/compressed/vmlinux.lds.S | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index 81c493156ce8..1fa62432e283 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -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;
-- 
2.9.3

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

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

Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/compressed/efi-header.S  | 32 ++++++++++++++------
 arch/arm/boot/compressed/vmlinux.lds.S | 30 +++++++++++++-----
 2 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index 542e1ad432ae..c94a88ae834d 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -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
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index 1fa62432e283..dfcc2baa0077 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -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) }
-- 
2.9.3

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

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

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/boot/compressed/efi-header.S  | 32 ++++++++++++++------
 arch/arm/boot/compressed/vmlinux.lds.S | 30 +++++++++++++-----
 2 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index 542e1ad432ae..c94a88ae834d 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -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
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index 1fa62432e283..dfcc2baa0077 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -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) }
-- 
2.9.3

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

[Gregory CLEMENT]

Hi Ard,
 
 On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> 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

>
> Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/boot/compressed/efi-header.S  | 32 ++++++++++++++------
>  arch/arm/boot/compressed/vmlinux.lds.S | 30 +++++++++++++-----
>  2 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
> index 542e1ad432ae..c94a88ae834d 100644
> --- a/arch/arm/boot/compressed/efi-header.S
> +++ b/arch/arm/boot/compressed/efi-header.S
> @@ -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
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index 1fa62432e283..dfcc2baa0077 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -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) }
> -- 
> 2.9.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Gregory CLEMENT]

Hi Ard,
 
 On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> 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

>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/boot/compressed/efi-header.S  | 32 ++++++++++++++------
>  arch/arm/boot/compressed/vmlinux.lds.S | 30 +++++++++++++-----
>  2 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
> index 542e1ad432ae..c94a88ae834d 100644
> --- a/arch/arm/boot/compressed/efi-header.S
> +++ b/arch/arm/boot/compressed/efi-header.S
> @@ -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
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index 1fa62432e283..dfcc2baa0077 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -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) }
> -- 
> 2.9.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Ard Biesheuvel]

On 8 September 2017 at 14:50, Gregory CLEMENT
<gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Ard,
>
>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> 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/
>

Ouch.

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

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

[Ard Biesheuvel]

On 8 September 2017 at 14:50, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Ard,
>
>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> 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/
>

Ouch.

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

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

[Ard Biesheuvel]

On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 8 September 2017 at 14:50, Gregory CLEMENT
> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> Hi Ard,
>>
>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>>> 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/
>>
>
> Ouch.
>
>> 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.

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)


diff --git a/arch/arm/boot/compressed/efi-header.S
b/arch/arm/boot/compressed/efi-header.S
index c94a88ae834d..671a6e5b7b99 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -127,7 +127,7 @@ section_table:

                .set    section_count, (. - section_table) / 40

-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm

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

[Ard Biesheuvel]

On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 September 2017 at 14:50, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> Hi Ard,
>>
>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>>> 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/
>>
>
> Ouch.
>
>> 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.

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)


diff --git a/arch/arm/boot/compressed/efi-header.S
b/arch/arm/boot/compressed/efi-header.S
index c94a88ae834d..671a6e5b7b99 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -127,7 +127,7 @@ section_table:

                .set    section_count, (. - section_table) / 40

-               .align  12
+               .align  9
 __efi_start:
 #endif
                .endm

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

[Gregory CLEMENT]

Hi Ard,
 
 On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 8 September 2017 at 14:50, Gregory CLEMENT
>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>> Hi Ard,
>>>
>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>
>>>> 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/
>>>
>>
>> Ouch.
>>
>>> 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.
>
> 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

>
>
> diff --git a/arch/arm/boot/compressed/efi-header.S
> b/arch/arm/boot/compressed/efi-header.S
> index c94a88ae834d..671a6e5b7b99 100644
> --- a/arch/arm/boot/compressed/efi-header.S
> +++ b/arch/arm/boot/compressed/efi-header.S
> @@ -127,7 +127,7 @@ section_table:
>
>                 .set    section_count, (. - section_table) / 40
>
> -               .align  12
> +               .align  9
>  __efi_start:
>  #endif
>                 .endm

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Gregory CLEMENT]

Hi Ard,
 
 On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 8 September 2017 at 14:50, Gregory CLEMENT
>> <gregory.clement@free-electrons.com> wrote:
>>> Hi Ard,
>>>
>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>
>>>> 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/
>>>
>>
>> Ouch.
>>
>>> 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.
>
> 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

>
>
> diff --git a/arch/arm/boot/compressed/efi-header.S
> b/arch/arm/boot/compressed/efi-header.S
> index c94a88ae834d..671a6e5b7b99 100644
> --- a/arch/arm/boot/compressed/efi-header.S
> +++ b/arch/arm/boot/compressed/efi-header.S
> @@ -127,7 +127,7 @@ section_table:
>
>                 .set    section_count, (. - section_table) / 40
>
> -               .align  12
> +               .align  9
>  __efi_start:
>  #endif
>                 .endm

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Ard Biesheuvel]

On 8 September 2017 at 15:33, Gregory CLEMENT
<gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Ard,
>
>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>> Hi Ard,
>>>>
>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>
>>>>> 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/
>>>>
>>>
>>> Ouch.
>>>
>>>> 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.
>>
>> 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
>
>>
>>
>> diff --git a/arch/arm/boot/compressed/efi-header.S
>> b/arch/arm/boot/compressed/efi-header.S
>> index c94a88ae834d..671a6e5b7b99 100644
>> --- a/arch/arm/boot/compressed/efi-header.S
>> +++ b/arch/arm/boot/compressed/efi-header.S
>> @@ -127,7 +127,7 @@ section_table:
>>
>>                 .set    section_count, (. - section_table) / 40
>>
>> -               .align  12
>> +               .align  9
>>  __efi_start:
>>  #endif
>>                 .endm
>


How about this?

diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
b/arch/arm/boot/compressed/vmlinux.lds.S
index 7a4c59154361..dfcc2baa0077 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -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;

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

[Ard Biesheuvel]

On 8 September 2017 at 15:33, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Ard,
>
>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>> <gregory.clement@free-electrons.com> wrote:
>>>> Hi Ard,
>>>>
>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>
>>>>> 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/
>>>>
>>>
>>> Ouch.
>>>
>>>> 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.
>>
>> 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
>
>>
>>
>> diff --git a/arch/arm/boot/compressed/efi-header.S
>> b/arch/arm/boot/compressed/efi-header.S
>> index c94a88ae834d..671a6e5b7b99 100644
>> --- a/arch/arm/boot/compressed/efi-header.S
>> +++ b/arch/arm/boot/compressed/efi-header.S
>> @@ -127,7 +127,7 @@ section_table:
>>
>>                 .set    section_count, (. - section_table) / 40
>>
>> -               .align  12
>> +               .align  9
>>  __efi_start:
>>  #endif
>>                 .endm
>


How about this?

diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
b/arch/arm/boot/compressed/vmlinux.lds.S
index 7a4c59154361..dfcc2baa0077 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -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;

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

[Gregory CLEMENT]

Hi Ard,
 
 On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 8 September 2017 at 15:33, Gregory CLEMENT
> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> Hi Ard,
>>
>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>> Hi Ard,
>>>>>
>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>
>>>>>> 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/
>>>>>
>>>>
>>>> Ouch.
>>>>
>>>>> 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.
>>>
>>> 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
>>
>>>
>>>
>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>> b/arch/arm/boot/compressed/efi-header.S
>>> index c94a88ae834d..671a6e5b7b99 100644
>>> --- a/arch/arm/boot/compressed/efi-header.S
>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>> @@ -127,7 +127,7 @@ section_table:
>>>
>>>                 .set    section_count, (. - section_table) / 40
>>>
>>> -               .align  12
>>> +               .align  9
>>>  __efi_start:
>>>  #endif
>>>                 .endm
>>
>
>
> 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:
Tested-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks,

Gregory


>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..dfcc2baa0077 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -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-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Gregory CLEMENT]

Hi Ard,
 
 On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> On 8 September 2017 at 15:33, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> Hi Ard,
>>
>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>> <gregory.clement@free-electrons.com> wrote:
>>>>> Hi Ard,
>>>>>
>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>
>>>>>> 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/
>>>>>
>>>>
>>>> Ouch.
>>>>
>>>>> 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.
>>>
>>> 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
>>
>>>
>>>
>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>> b/arch/arm/boot/compressed/efi-header.S
>>> index c94a88ae834d..671a6e5b7b99 100644
>>> --- a/arch/arm/boot/compressed/efi-header.S
>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>> @@ -127,7 +127,7 @@ section_table:
>>>
>>>                 .set    section_count, (. - section_table) / 40
>>>
>>> -               .align  12
>>> +               .align  9
>>>  __efi_start:
>>>  #endif
>>>                 .endm
>>
>
>
> 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:
Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory


>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..dfcc2baa0077 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Ard Biesheuvel]

On 8 September 2017 at 15:56, Gregory CLEMENT
<gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Ard,
>
>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> On 8 September 2017 at 15:33, Gregory CLEMENT
>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>> Hi Ard,
>>>
>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>
>>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>>> Hi Ard,
>>>>>>
>>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>
>>>>>>> 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/
>>>>>>
>>>>>
>>>>> Ouch.
>>>>>
>>>>>> 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.
>>>>
>>>> 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
>>>
>>>>
>>>>
>>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>>> b/arch/arm/boot/compressed/efi-header.S
>>>> index c94a88ae834d..671a6e5b7b99 100644
>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>> @@ -127,7 +127,7 @@ section_table:
>>>>
>>>>                 .set    section_count, (. - section_table) / 40
>>>>
>>>> -               .align  12
>>>> +               .align  9
>>>>  __efi_start:
>>>>  #endif
>>>>                 .endm
>>>
>>
>>
>> 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:
> Tested-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>
> Thanks,
>
> Gregory
>

Thanks a lot. I will send it out today.

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

[Ard Biesheuvel]

On 8 September 2017 at 15:56, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Ard,
>
>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> On 8 September 2017 at 15:33, Gregory CLEMENT
>> <gregory.clement@free-electrons.com> wrote:
>>> Hi Ard,
>>>
>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>
>>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>>> <gregory.clement@free-electrons.com> wrote:
>>>>>> Hi Ard,
>>>>>>
>>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>>
>>>>>>> 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/
>>>>>>
>>>>>
>>>>> Ouch.
>>>>>
>>>>>> 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.
>>>>
>>>> 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
>>>
>>>>
>>>>
>>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>>> b/arch/arm/boot/compressed/efi-header.S
>>>> index c94a88ae834d..671a6e5b7b99 100644
>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>> @@ -127,7 +127,7 @@ section_table:
>>>>
>>>>                 .set    section_count, (. - section_table) / 40
>>>>
>>>> -               .align  12
>>>> +               .align  9
>>>>  __efi_start:
>>>>  #endif
>>>>                 .endm
>>>
>>
>>
>> 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:
> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> Thanks,
>
> Gregory
>

Thanks a lot. I will send it out today.

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

[Ard Biesheuvel]

On 8 September 2017 at 15:57, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 8 September 2017 at 15:56, Gregory CLEMENT
> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> Hi Ard,
>>
>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>>> On 8 September 2017 at 15:33, Gregory CLEMENT
>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>> Hi Ard,
>>>>
>>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>
>>>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>>>> Hi Ard,
>>>>>>>
>>>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>>
>>>>>>>> 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/
>>>>>>>
>>>>>>
>>>>>> Ouch.
>>>>>>
>>>>>>> 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.
>>>>>
>>>>> 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
>>>>
>>>>>
>>>>>
>>>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>>>> b/arch/arm/boot/compressed/efi-header.S
>>>>> index c94a88ae834d..671a6e5b7b99 100644
>>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>>> @@ -127,7 +127,7 @@ section_table:
>>>>>
>>>>>                 .set    section_count, (. - section_table) / 40
>>>>>
>>>>> -               .align  12
>>>>> +               .align  9
>>>>>  __efi_start:
>>>>>  #endif
>>>>>                 .endm
>>>>
>>>
>>>
>>> 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:
>> Tested-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>

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

diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S
index f72088495f43..5d52c556dd32 100644
--- a/arch/arm/boot/compressed/piggy.S
+++ b/arch/arm/boot/compressed/piggy.S
@@ -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.

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

[Ard Biesheuvel]

On 8 September 2017 at 15:57, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 September 2017 at 15:56, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> Hi Ard,
>>
>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>>> On 8 September 2017 at 15:33, Gregory CLEMENT
>>> <gregory.clement@free-electrons.com> wrote:
>>>> Hi Ard,
>>>>
>>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>
>>>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>>>> <gregory.clement@free-electrons.com> wrote:
>>>>>>> Hi Ard,
>>>>>>>
>>>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>>>
>>>>>>>> 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/
>>>>>>>
>>>>>>
>>>>>> Ouch.
>>>>>>
>>>>>>> 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.
>>>>>
>>>>> 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
>>>>
>>>>>
>>>>>
>>>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>>>> b/arch/arm/boot/compressed/efi-header.S
>>>>> index c94a88ae834d..671a6e5b7b99 100644
>>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>>> @@ -127,7 +127,7 @@ section_table:
>>>>>
>>>>>                 .set    section_count, (. - section_table) / 40
>>>>>
>>>>> -               .align  12
>>>>> +               .align  9
>>>>>  __efi_start:
>>>>>  #endif
>>>>>                 .endm
>>>>
>>>
>>>
>>> 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:
>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>

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

diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S
index f72088495f43..5d52c556dd32 100644
--- a/arch/arm/boot/compressed/piggy.S
+++ b/arch/arm/boot/compressed/piggy.S
@@ -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.

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

[Gregory CLEMENT]

Hi Ard,
 
 On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 8 September 2017 at 15:57, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 8 September 2017 at 15:56, Gregory CLEMENT
>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>> Hi Ard,
>>>
>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>
>>>> On 8 September 2017 at 15:33, Gregory CLEMENT
>>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>> Hi Ard,
>>>>>
>>>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>
>>>>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>>>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>>>>> Hi Ard,
>>>>>>>>
>>>>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>>>
>>>>>>>>> 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/
>>>>>>>>
>>>>>>>
>>>>>>> Ouch.
>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> 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
>>>>>
>>>>>>
>>>>>>
>>>>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>>>>> b/arch/arm/boot/compressed/efi-header.S
>>>>>> index c94a88ae834d..671a6e5b7b99 100644
>>>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>>>> @@ -127,7 +127,7 @@ section_table:
>>>>>>
>>>>>>                 .set    section_count, (. - section_table) / 40
>>>>>>
>>>>>> -               .align  12
>>>>>> +               .align  9
>>>>>>  __efi_start:
>>>>>>  #endif
>>>>>>                 .endm
>>>>>
>>>>
>>>>
>>>> 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:
>>> Tested-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>>
>
> Would you mind checking whether this fixes the issue as well?
>
> diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S
> index f72088495f43..5d52c556dd32 100644
> --- a/arch/arm/boot/compressed/piggy.S
> +++ b/arch/arm/boot/compressed/piggy.S
> @@ -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.

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

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Gregory CLEMENT]

Hi Ard,
 
 On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> On 8 September 2017 at 15:57, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 8 September 2017 at 15:56, Gregory CLEMENT
>> <gregory.clement@free-electrons.com> wrote:
>>> Hi Ard,
>>>
>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>
>>>> On 8 September 2017 at 15:33, Gregory CLEMENT
>>>> <gregory.clement@free-electrons.com> wrote:
>>>>> Hi Ard,
>>>>>
>>>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>
>>>>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>>>>> <gregory.clement@free-electrons.com> wrote:
>>>>>>>> Hi Ard,
>>>>>>>>
>>>>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>>>>
>>>>>>>>> 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/
>>>>>>>>
>>>>>>>
>>>>>>> Ouch.
>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> 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
>>>>>
>>>>>>
>>>>>>
>>>>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>>>>> b/arch/arm/boot/compressed/efi-header.S
>>>>>> index c94a88ae834d..671a6e5b7b99 100644
>>>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>>>> @@ -127,7 +127,7 @@ section_table:
>>>>>>
>>>>>>                 .set    section_count, (. - section_table) / 40
>>>>>>
>>>>>> -               .align  12
>>>>>> +               .align  9
>>>>>>  __efi_start:
>>>>>>  #endif
>>>>>>                 .endm
>>>>>
>>>>
>>>>
>>>> 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:
>>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>>
>
> Would you mind checking whether this fixes the issue as well?
>
> diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S
> index f72088495f43..5d52c556dd32 100644
> --- a/arch/arm/boot/compressed/piggy.S
> +++ b/arch/arm/boot/compressed/piggy.S
> @@ -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.

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

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Ard Biesheuvel]

On 8 September 2017 at 16:17, Gregory CLEMENT
<gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Ard,
>
>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> On 8 September 2017 at 15:57, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> On 8 September 2017 at 15:56, Gregory CLEMENT
>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>> Hi Ard,
>>>>
>>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>
>>>>> On 8 September 2017 at 15:33, Gregory CLEMENT
>>>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>>> Hi Ard,
>>>>>>
>>>>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>
>>>>>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>>>>>> <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>>>>>> Hi Ard,
>>>>>>>>>
>>>>>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>>>>
>>>>>>>>>> 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/
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ouch.
>>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>> 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
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>>>>>> b/arch/arm/boot/compressed/efi-header.S
>>>>>>> index c94a88ae834d..671a6e5b7b99 100644
>>>>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>>>>> @@ -127,7 +127,7 @@ section_table:
>>>>>>>
>>>>>>>                 .set    section_count, (. - section_table) / 40
>>>>>>>
>>>>>>> -               .align  12
>>>>>>> +               .align  9
>>>>>>>  __efi_start:
>>>>>>>  #endif
>>>>>>>                 .endm
>>>>>>
>>>>>
>>>>>
>>>>> 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:
>>>> Tested-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>>>
>>
>> Would you mind checking whether this fixes the issue as well?
>>
>> diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S
>> index f72088495f43..5d52c556dd32 100644
>> --- a/arch/arm/boot/compressed/piggy.S
>> +++ b/arch/arm/boot/compressed/piggy.S
>> @@ -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.
>
> 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.

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

[Ard Biesheuvel]

On 8 September 2017 at 16:17, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Ard,
>
>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> On 8 September 2017 at 15:57, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 8 September 2017 at 15:56, Gregory CLEMENT
>>> <gregory.clement@free-electrons.com> wrote:
>>>> Hi Ard,
>>>>
>>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>
>>>>> On 8 September 2017 at 15:33, Gregory CLEMENT
>>>>> <gregory.clement@free-electrons.com> wrote:
>>>>>> Hi Ard,
>>>>>>
>>>>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>>
>>>>>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>>>>>> <gregory.clement@free-electrons.com> wrote:
>>>>>>>>> Hi Ard,
>>>>>>>>>
>>>>>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>>> 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/
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ouch.
>>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>> 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
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/compressed/efi-header.S
>>>>>>> b/arch/arm/boot/compressed/efi-header.S
>>>>>>> index c94a88ae834d..671a6e5b7b99 100644
>>>>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>>>>> @@ -127,7 +127,7 @@ section_table:
>>>>>>>
>>>>>>>                 .set    section_count, (. - section_table) / 40
>>>>>>>
>>>>>>> -               .align  12
>>>>>>> +               .align  9
>>>>>>>  __efi_start:
>>>>>>>  #endif
>>>>>>>                 .endm
>>>>>>
>>>>>
>>>>>
>>>>> 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:
>>>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>>>
>>
>> Would you mind checking whether this fixes the issue as well?
>>
>> diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S
>> index f72088495f43..5d52c556dd32 100644
>> --- a/arch/arm/boot/compressed/piggy.S
>> +++ b/arch/arm/boot/compressed/piggy.S
>> @@ -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.
>
> 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.

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

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

Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/compressed/efi-header.S  | 1 +
 arch/arm/boot/compressed/vmlinux.lds.S | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index c94a88ae834d..a14516f7a27e 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -129,5 +129,6 @@ section_table:
 
 		.align	12
 __efi_start:
+		.text
 #endif
 		.endm
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index dfcc2baa0077..aa0ee9b5affe 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -39,9 +39,11 @@ SECTIONS
   . = TEXT_START;
   _text = .;
 
-  .text : {
+  .start : {
     _start = .;
     *(.start)
+  }
+  .text : {
     *(.text)
     *(.text.*)
     *(.fixup)
-- 
2.9.3

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

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

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/boot/compressed/efi-header.S  | 1 +
 arch/arm/boot/compressed/vmlinux.lds.S | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index c94a88ae834d..a14516f7a27e 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -129,5 +129,6 @@ section_table:
 
 		.align	12
 __efi_start:
+		.text
 #endif
 		.endm
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index dfcc2baa0077..aa0ee9b5affe 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -39,9 +39,11 @@ SECTIONS
   . = TEXT_START;
   _text = .;
 
-  .text : {
+  .start : {
     _start = .;
     *(.start)
+  }
+  .text : {
     *(.text)
     *(.text.*)
     *(.fixup)
-- 
2.9.3

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

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

Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/compressed/Makefile     |  4 ++
 arch/arm/boot/compressed/efi-header.S | 47 ++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index d50430c40045..6b978bdbac3e 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -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
diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index a14516f7a27e..e902414e90b6 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -98,6 +98,11 @@ extra_header_fields:
 		.quad	0				@ CertificationTable
 		.quad	0				@ BaseRelocationTable
 
+#ifdef CONFIG_DEBUG_INFO
+		.long	efi_debug_table - start		@ DebugTable
+		.long	efi_debug_table_size
+#endif
+
 section_table:
 		.ascii	".text\0\0\0"
 		.long	__pecoff_code_size		@ VirtualSize
@@ -127,6 +132,48 @@ section_table:
 
 		.set	section_count, (. - section_table) / 40
 
+#ifdef CONFIG_DEBUG_INFO
+		/*
+		 * The debug table is referenced via its Relative Virtual
+		 * Address (RVA), which is only defined for those parts of
+		 * the image that are covered by a section declaration. Since
+		 * this header is not covered by any section, the debug table
+		 * must be emitted elsewhere. So stick it in the .rodata
+		 * section instead.
+		 *
+		 * Note that the EFI debug entry itself may legally have a
+		 * zero RVA, which means we can simply put it right after the
+		 * section headers.
+		 */
+		.section	".rodata", #alloc
+
+		.align	2
+efi_debug_table:
+		// EFI_IMAGE_DEBUG_DIRECTORY_ENTRY
+		.long	0				@ Characteristics
+		.long	0				@ TimeDateStamp
+		.short	0				@ MajorVersion
+		.short	0				@ MinorVersion
+		.long	IMAGE_DEBUG_TYPE_CODEVIEW	@ Type
+		.long	efi_debug_entry_size		@ SizeOfData
+		.long	0				@ RVA
+		.long	efi_debug_entry - start		@ FileOffset
+
+		.set	efi_debug_table_size, . - efi_debug_table
+		.previous
+
+efi_debug_entry:
+		// EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY
+		.ascii	"NB10"				@ Signature
+		.long	0				@ Unknown
+		.long	0				@ Unknown2
+		.long	0				@ Unknown3
+
+		.asciz	VMLINUX_PATH
+
+		.set	efi_debug_entry_size, . - efi_debug_entry
+#endif
+
 		.align	12
 __efi_start:
 		.text
-- 
2.9.3

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

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

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/boot/compressed/Makefile     |  4 ++
 arch/arm/boot/compressed/efi-header.S | 47 ++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index d50430c40045..6b978bdbac3e 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -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
diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index a14516f7a27e..e902414e90b6 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -98,6 +98,11 @@ extra_header_fields:
 		.quad	0				@ CertificationTable
 		.quad	0				@ BaseRelocationTable
 
+#ifdef CONFIG_DEBUG_INFO
+		.long	efi_debug_table - start		@ DebugTable
+		.long	efi_debug_table_size
+#endif
+
 section_table:
 		.ascii	".text\0\0\0"
 		.long	__pecoff_code_size		@ VirtualSize
@@ -127,6 +132,48 @@ section_table:
 
 		.set	section_count, (. - section_table) / 40
 
+#ifdef CONFIG_DEBUG_INFO
+		/*
+		 * The debug table is referenced via its Relative Virtual
+		 * Address (RVA), which is only defined for those parts of
+		 * the image that are covered by a section declaration. Since
+		 * this header is not covered by any section, the debug table
+		 * must be emitted elsewhere. So stick it in the .rodata
+		 * section instead.
+		 *
+		 * Note that the EFI debug entry itself may legally have a
+		 * zero RVA, which means we can simply put it right after the
+		 * section headers.
+		 */
+		.section	".rodata", #alloc
+
+		.align	2
+efi_debug_table:
+		// EFI_IMAGE_DEBUG_DIRECTORY_ENTRY
+		.long	0				@ Characteristics
+		.long	0				@ TimeDateStamp
+		.short	0				@ MajorVersion
+		.short	0				@ MinorVersion
+		.long	IMAGE_DEBUG_TYPE_CODEVIEW	@ Type
+		.long	efi_debug_entry_size		@ SizeOfData
+		.long	0				@ RVA
+		.long	efi_debug_entry - start		@ FileOffset
+
+		.set	efi_debug_table_size, . - efi_debug_table
+		.previous
+
+efi_debug_entry:
+		// EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY
+		.ascii	"NB10"				@ Signature
+		.long	0				@ Unknown
+		.long	0				@ Unknown2
+		.long	0				@ Unknown3
+
+		.asciz	VMLINUX_PATH
+
+		.set	efi_debug_entry_size, . - efi_debug_entry
+#endif
+
 		.align	12
 __efi_start:
 		.text
-- 
2.9.3