[RFC PATCH 0/3] efi: put an API version number in the PE/COFF header

[RFC PATCH 0/3] efi: put an API version number in the PE/COFF header

[Ard Biesheuvel]

After having added new ways to load the initrd and the mixed mode kernel,
we will need to tell the loader about this, so it can use the new, generic
method if supported, and fall back to the existing method(s) otherwise.

This is an RFC - if there are better ways to record this in the image
somehow, please shout.

Cc: lersek@redhat.com
Cc: leif@nuviainc.com
Cc: pjones@redhat.com
Cc: mjg59@google.com
Cc: agraf@csgraf.de
Cc: ilias.apalodimas@linaro.org
Cc: xypron.glpk@gmx.de
Cc: daniel.kiper@oracle.com
Cc: nivedita@alum.mit.edu
Cc: James.Bottomley@hansenpartnership.com

Ard Biesheuvel (3):
  efi/x86: Use symbolic constants in PE header instead of bare numbers
  efi/libstub: Introduce symbolic constants for the stub major/minor
    version
  efi: Bump the Linux EFI stub major version number to #1

 arch/arm/boot/compressed/efi-header.S |  4 +-
 arch/arm64/kernel/efi-header.S        |  4 +-
 arch/x86/boot/header.S                | 64 ++++++++++----------
 include/linux/pe.h                    | 21 +++++++
 4 files changed, 58 insertions(+), 35 deletions(-)

-- 
2.17.1

[RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Ard Biesheuvel]

Replace bare numbers in the PE/COFF header structure with symbolic
constants so they become self documenting.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/header.S | 60 ++++++++++----------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 44aeb63ca6ae..9110b58aa2ec 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -15,7 +15,7 @@
  * hex while segment addresses are written as segment:offset.
  *
  */
-
+#include <linux/pe.h>
 #include <asm/segment.h>
 #include <asm/boot.h>
 #include <asm/page_types.h>
@@ -43,8 +43,7 @@ SYSSEG		= 0x1000		/* historical load address >> 4 */
 bootsect_start:
 #ifdef CONFIG_EFI_STUB
 	# "MZ", MS-DOS header
-	.byte 0x4d
-	.byte 0x5a
+	.word	MZ_MAGIC
 #endif
 
 	# Normalize the start address
@@ -97,39 +96,30 @@ bugger_off_msg:
 
 #ifdef CONFIG_EFI_STUB
 pe_header:
-	.ascii	"PE"
-	.word 	0
+	.long	PE_MAGIC
 
 coff_header:
 #ifdef CONFIG_X86_32
-	.word	0x14c				# i386
+	.set	image_file_add_flags, IMAGE_FILE_32BIT_MACHINE
+	.set	pe_opt_magic, PE_OPT_MAGIC_PE32
+	.word	IMAGE_FILE_MACHINE_I386
 #else
-	.word	0x8664				# x86-64
+	.set	image_file_add_flags, 0
+	.set	pe_opt_magic, PE_OPT_MAGIC_PE32PLUS
+	.word	IMAGE_FILE_MACHINE_AMD64
 #endif
 	.word	section_count			# nr_sections
 	.long	0 				# TimeDateStamp
 	.long	0				# PointerToSymbolTable
 	.long	1				# NumberOfSymbols
 	.word	section_table - optional_header	# SizeOfOptionalHeader
-#ifdef CONFIG_X86_32
-	.word	0x306				# Characteristics.
-						# IMAGE_FILE_32BIT_MACHINE |
-						# IMAGE_FILE_DEBUG_STRIPPED |
-						# IMAGE_FILE_EXECUTABLE_IMAGE |
-						# IMAGE_FILE_LINE_NUMS_STRIPPED
-#else
-	.word	0x206				# Characteristics
-						# IMAGE_FILE_DEBUG_STRIPPED |
-						# IMAGE_FILE_EXECUTABLE_IMAGE |
-						# IMAGE_FILE_LINE_NUMS_STRIPPED
-#endif
+	.word	IMAGE_FILE_EXECUTABLE_IMAGE	| \
+		image_file_add_flags		| \
+		IMAGE_FILE_DEBUG_STRIPPED	| \
+		IMAGE_FILE_LINE_NUMS_STRIPPED	# Characteristics
 
 optional_header:
-#ifdef CONFIG_X86_32
-	.word	0x10b				# PE32 format
-#else
-	.word	0x20b 				# PE32+ format
-#endif
+	.word	pe_opt_magic
 	.byte	0x02				# MajorLinkerVersion
 	.byte	0x14				# MinorLinkerVersion
 
@@ -170,7 +160,7 @@ extra_header_fields:
 
 	.long	0x200				# SizeOfHeaders
 	.long	0				# CheckSum
-	.word	0xa				# Subsystem (EFI application)
+	.word	IMAGE_SUBSYSTEM_EFI_APPLICATION	# Subsystem (EFI application)
 	.word	0				# DllCharacteristics
 #ifdef CONFIG_X86_32
 	.long	0				# SizeOfStackReserve
@@ -210,7 +200,10 @@ section_table:
 	.long	0				# PointerToLineNumbers
 	.word	0				# NumberOfRelocations
 	.word	0				# NumberOfLineNumbers
-	.long	0x60500020			# Characteristics (section flags)
+	.long	IMAGE_SCN_CNT_CODE		| \
+		IMAGE_SCN_MEM_READ		| \
+		IMAGE_SCN_MEM_EXECUTE		| \
+		IMAGE_SCN_ALIGN_16BYTES		# Characteristics
 
 	#
 	# The EFI application loader requires a relocation section
@@ -228,7 +221,10 @@ section_table:
 	.long	0				# PointerToLineNumbers
 	.word	0				# NumberOfRelocations
 	.word	0				# NumberOfLineNumbers
-	.long	0x42100040			# Characteristics (section flags)
+	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
+		IMAGE_SCN_MEM_READ		| \
+		IMAGE_SCN_MEM_DISCARDABLE	| \
+		IMAGE_SCN_ALIGN_1BYTES		# Characteristics
 
 #ifdef CONFIG_EFI_MIXED
 	#
@@ -244,7 +240,10 @@ section_table:
 	.long	0				# PointerToLineNumbers
 	.word	0				# NumberOfRelocations
 	.word	0				# NumberOfLineNumbers
-	.long	0x42100040			# Characteristics (section flags)
+	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
+		IMAGE_SCN_MEM_READ		| \
+		IMAGE_SCN_MEM_DISCARDABLE	| \
+		IMAGE_SCN_ALIGN_1BYTES		# Characteristics
 #endif
 
 	#
@@ -263,7 +262,10 @@ section_table:
 	.long	0				# PointerToLineNumbers
 	.word	0				# NumberOfRelocations
 	.word	0				# NumberOfLineNumbers
-	.long	0x60500020			# Characteristics (section flags)
+	.long	IMAGE_SCN_CNT_CODE		| \
+		IMAGE_SCN_MEM_READ		| \
+		IMAGE_SCN_MEM_EXECUTE		| \
+		IMAGE_SCN_ALIGN_16BYTES		# Characteristics
 
 	.set	section_count, (. - section_table) / 40
 #endif /* CONFIG_EFI_STUB */
-- 
2.17.1

[RFC PATCH 2/3] efi/libstub: Introduce symbolic constants for the stub major/minor version

[Ard Biesheuvel]

Now that we have added new ways to load the initrd or the mixed mode
kernel, we will also need a way to tell the loader about this. Add
symbolic constants for the PE/COFF major/minor version numbers (which
fortunately have always been 0x0 for all architectures), so that we
can bump them later to document the capabilities of the stub.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm/boot/compressed/efi-header.S | 4 ++--
 arch/arm64/kernel/efi-header.S        | 4 ++--
 arch/x86/boot/header.S                | 4 ++--
 include/linux/pe.h                    | 3 +++
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index e38fbda02b93..62286da318e7 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -70,8 +70,8 @@ extra_header_fields:
 		.long	SZ_512				@ FileAlignment
 		.short	0				@ MajorOsVersion
 		.short	0				@ MinorOsVersion
-		.short	0				@ MajorImageVersion
-		.short	0				@ MinorImageVersion
+		.short	LINUX_EFISTUB_MAJOR_VERSION	@ MajorImageVersion
+		.short	LINUX_EFISTUB_MINOR_VERSION	@ MinorImageVersion
 		.short	0				@ MajorSubsystemVersion
 		.short	0				@ MinorSubsystemVersion
 		.long	0				@ Win32VersionValue
diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
index 40c704c5d3a5..914999ccaf8a 100644
--- a/arch/arm64/kernel/efi-header.S
+++ b/arch/arm64/kernel/efi-header.S
@@ -36,8 +36,8 @@ extra_header_fields:
 	.long	PECOFF_FILE_ALIGNMENT			// FileAlignment
 	.short	0					// MajorOperatingSystemVersion
 	.short	0					// MinorOperatingSystemVersion
-	.short	0					// MajorImageVersion
-	.short	0					// MinorImageVersion
+	.short	LINUX_EFISTUB_MAJOR_VERSION		// MajorImageVersion
+	.short	LINUX_EFISTUB_MINOR_VERSION		// MinorImageVersion
 	.short	0					// MajorSubsystemVersion
 	.short	0					// MinorSubsystemVersion
 	.long	0					// Win32VersionValue
diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 9110b58aa2ec..a87d788ea54e 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -147,8 +147,8 @@ extra_header_fields:
 	.long	0x20				# FileAlignment
 	.word	0				# MajorOperatingSystemVersion
 	.word	0				# MinorOperatingSystemVersion
-	.word	0				# MajorImageVersion
-	.word	0				# MinorImageVersion
+	.word	LINUX_EFISTUB_MAJOR_VERSION	# MajorImageVersion
+	.word	LINUX_EFISTUB_MINOR_VERSION	# MinorImageVersion
 	.word	0				# MajorSubsystemVersion
 	.word	0				# MinorSubsystemVersion
 	.long	0				# Win32VersionValue
diff --git a/include/linux/pe.h b/include/linux/pe.h
index c86bd3a2f70f..e0869f3eadd6 100644
--- a/include/linux/pe.h
+++ b/include/linux/pe.h
@@ -10,6 +10,9 @@
 
 #include <linux/types.h>
 
+#define LINUX_EFISTUB_MAJOR_VERSION		0x0
+#define LINUX_EFISTUB_MINOR_VERSION		0x0
+
 #define MZ_MAGIC	0x5a4d	/* "MZ" */
 
 #define PE_MAGIC		0x00004550	/* "PE\0\0" */
-- 
2.17.1

[RFC PATCH 3/3] efi: Bump the Linux EFI stub major version number to #1

[Ard Biesheuvel]

Now that we have introduced new, generic ways for the OS loader to
interface with Linux kernels during boot, we need to record this
fact in a way that allows loaders to discover this information, and
fall back to the existing methods for older kernels.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 include/linux/pe.h | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/include/linux/pe.h b/include/linux/pe.h
index e0869f3eadd6..8ad71d763a77 100644
--- a/include/linux/pe.h
+++ b/include/linux/pe.h
@@ -10,7 +10,25 @@
 
 #include <linux/types.h>
 
-#define LINUX_EFISTUB_MAJOR_VERSION		0x0
+/*
+ * Linux EFI stub v1.0 adds the following functionality:
+ * - Loading initrd from the LINUX_EFI_INITRD_MEDIA_GUID device path,
+ * - Loading/starting the kernel from firmware that targets a different
+ *   machine type, via the entrypoint exposed in the .compat PE/COFF section.
+ *
+ * The recommended way of loading and starting v1.0 or later kernels is to use
+ * the LoadImage() and StartImage() EFI boot services, and expose the initrd
+ * via the LINUX_EFI_INITRD_MEDIA_GUID device path.
+ *
+ * Versions older than v1.0 support initrd loading via the image load options
+ * (using initrd=, limited to the volume from which the kernel itself was
+ * loaded), or via arch specific means (bootparams, DT, etc).
+ *
+ * On x86, LoadImage() and StartImage() can be omitted if the EFI handover
+ * protocol is implemented, which can be inferred from the version,
+ * handover_offset and xloadflags fields in the bootparams structure.
+ */
+#define LINUX_EFISTUB_MAJOR_VERSION		0x1
 #define LINUX_EFISTUB_MINOR_VERSION		0x0
 
 #define MZ_MAGIC	0x5a4d	/* "MZ" */
-- 
2.17.1

Re: [RFC PATCH 0/3] efi: put an API version number in the PE/COFF header

[Daniel Kiper]

On Thu, Feb 20, 2020 at 12:06:46PM +0100, Ard Biesheuvel wrote:
> After having added new ways to load the initrd and the mixed mode kernel,
> we will need to tell the loader about this, so it can use the new, generic
> method if supported, and fall back to the existing method(s) otherwise.
>
> This is an RFC - if there are better ways to record this in the image
> somehow, please shout.
>
> Cc: lersek@redhat.com
> Cc: leif@nuviainc.com
> Cc: pjones@redhat.com
> Cc: mjg59@google.com
> Cc: agraf@csgraf.de
> Cc: ilias.apalodimas@linaro.org
> Cc: xypron.glpk@gmx.de
> Cc: daniel.kiper@oracle.com
> Cc: nivedita@alum.mit.edu
> Cc: James.Bottomley@hansenpartnership.com

For all the patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

I hope that we will see a bootloader, GRUB preferred, implementation for
this thing soon...

Daniel

Re: [RFC PATCH 0/3] efi: put an API version number in the PE/COFF header

[Heinrich Schuchardt]

On 2/20/20 12:06 PM, Ard Biesheuvel wrote:
> After having added new ways to load the initrd and the mixed mode kernel,
> we will need to tell the loader about this, so it can use the new, generic
> method if supported, and fall back to the existing method(s) otherwise.
>
> This is an RFC - if there are better ways to record this in the image
> somehow, please shout.


Hello Ard,

for boot loaders like GRUB I understand that the boot loader could use
the initrd file path from its scripts to prepare a
EFI_LOAD_FILE2_PROTOCOL matching the loaded kernel.

I am not sure about the requirements for a firmware.

Up to now the U-Boot UEFI sub-system does not care about initial RAM
disk images at all. With Ilias suggested patch series U-Boot could offer
a file from a fixed file location via a EFI_LOAD_FILE2_PROTOCOL.

Is there anything else you expect a firmware like U-Boot or EDK2 to do?

Best regards

Heinrich

>
> Cc: lersek@redhat.com
> Cc: leif@nuviainc.com
> Cc: pjones@redhat.com
> Cc: mjg59@google.com
> Cc: agraf@csgraf.de
> Cc: ilias.apalodimas@linaro.org
> Cc: xypron.glpk@gmx.de
> Cc: daniel.kiper@oracle.com
> Cc: nivedita@alum.mit.edu
> Cc: James.Bottomley@hansenpartnership.com
>
> Ard Biesheuvel (3):
>    efi/x86: Use symbolic constants in PE header instead of bare numbers
>    efi/libstub: Introduce symbolic constants for the stub major/minor
>      version
>    efi: Bump the Linux EFI stub major version number to #1
>
>   arch/arm/boot/compressed/efi-header.S |  4 +-
>   arch/arm64/kernel/efi-header.S        |  4 +-
>   arch/x86/boot/header.S                | 64 ++++++++++----------
>   include/linux/pe.h                    | 21 +++++++
>   4 files changed, 58 insertions(+), 35 deletions(-)
>

Re: [RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Arvind Sankar]

On Thu, Feb 20, 2020 at 12:06:47PM +0100, Ard Biesheuvel wrote:
> Replace bare numbers in the PE/COFF header structure with symbolic
> constants so they become self documenting.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/boot/header.S | 60 ++++++++++----------
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 44aeb63ca6ae..9110b58aa2ec 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
> @@ -210,7 +200,10 @@ section_table:
>  	.long	0				# PointerToLineNumbers
>  	.word	0				# NumberOfRelocations
>  	.word	0				# NumberOfLineNumbers
> -	.long	0x60500020			# Characteristics (section flags)
> +	.long	IMAGE_SCN_CNT_CODE		| \
> +		IMAGE_SCN_MEM_READ		| \
> +		IMAGE_SCN_MEM_EXECUTE		| \
> +		IMAGE_SCN_ALIGN_16BYTES		# Characteristics

The IMAGE_SCN_ALIGN bits are marked as "valid only for object files".
Can they be removed?

>  
>  	#
>  	# The EFI application loader requires a relocation section
> @@ -228,7 +221,10 @@ section_table:
>  	.long	0				# PointerToLineNumbers
>  	.word	0				# NumberOfRelocations
>  	.word	0				# NumberOfLineNumbers
> -	.long	0x42100040			# Characteristics (section flags)
> +	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
> +		IMAGE_SCN_MEM_READ		| \
> +		IMAGE_SCN_MEM_DISCARDABLE	| \
> +		IMAGE_SCN_ALIGN_1BYTES		# Characteristics
>  
>  #ifdef CONFIG_EFI_MIXED
>  	#
> @@ -244,7 +240,10 @@ section_table:
>  	.long	0				# PointerToLineNumbers
>  	.word	0				# NumberOfRelocations
>  	.word	0				# NumberOfLineNumbers
> -	.long	0x42100040			# Characteristics (section flags)
> +	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
> +		IMAGE_SCN_MEM_READ		| \
> +		IMAGE_SCN_MEM_DISCARDABLE	| \
> +		IMAGE_SCN_ALIGN_1BYTES		# Characteristics
>  #endif
>  
>  	#
> @@ -263,7 +262,10 @@ section_table:
>  	.long	0				# PointerToLineNumbers
>  	.word	0				# NumberOfRelocations
>  	.word	0				# NumberOfLineNumbers
> -	.long	0x60500020			# Characteristics (section flags)
> +	.long	IMAGE_SCN_CNT_CODE		| \
> +		IMAGE_SCN_MEM_READ		| \
> +		IMAGE_SCN_MEM_EXECUTE		| \
> +		IMAGE_SCN_ALIGN_16BYTES		# Characteristics

The .text section contains data as well. Shouldn't it be marked
IMAGE_SCN_MEM_WRITE also?

>  
>  	.set	section_count, (. - section_table) / 40
>  #endif /* CONFIG_EFI_STUB */
> -- 
> 2.17.1
> 

Re: [RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Ard Biesheuvel]

On Thu, 20 Feb 2020 at 18:28, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Thu, Feb 20, 2020 at 12:06:47PM +0100, Ard Biesheuvel wrote:
> > Replace bare numbers in the PE/COFF header structure with symbolic
> > constants so they become self documenting.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/boot/header.S | 60 ++++++++++----------
> >  1 file changed, 31 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> > index 44aeb63ca6ae..9110b58aa2ec 100644
> > --- a/arch/x86/boot/header.S
> > +++ b/arch/x86/boot/header.S
> > @@ -210,7 +200,10 @@ section_table:
> >       .long   0                               # PointerToLineNumbers
> >       .word   0                               # NumberOfRelocations
> >       .word   0                               # NumberOfLineNumbers
> > -     .long   0x60500020                      # Characteristics (section flags)
> > +     .long   IMAGE_SCN_CNT_CODE              | \
> > +             IMAGE_SCN_MEM_READ              | \
> > +             IMAGE_SCN_MEM_EXECUTE           | \
> > +             IMAGE_SCN_ALIGN_16BYTES         # Characteristics
>
> The IMAGE_SCN_ALIGN bits are marked as "valid only for object files".
> Can they be removed?
>

Yes, they probably should. I just didn't want to do it in the same patch.

> >
> >       #
> >       # The EFI application loader requires a relocation section
> > @@ -228,7 +221,10 @@ section_table:
> >       .long   0                               # PointerToLineNumbers
> >       .word   0                               # NumberOfRelocations
> >       .word   0                               # NumberOfLineNumbers
> > -     .long   0x42100040                      # Characteristics (section flags)
> > +     .long   IMAGE_SCN_CNT_INITIALIZED_DATA  | \
> > +             IMAGE_SCN_MEM_READ              | \
> > +             IMAGE_SCN_MEM_DISCARDABLE       | \
> > +             IMAGE_SCN_ALIGN_1BYTES          # Characteristics
> >
> >  #ifdef CONFIG_EFI_MIXED
> >       #
> > @@ -244,7 +240,10 @@ section_table:
> >       .long   0                               # PointerToLineNumbers
> >       .word   0                               # NumberOfRelocations
> >       .word   0                               # NumberOfLineNumbers
> > -     .long   0x42100040                      # Characteristics (section flags)
> > +     .long   IMAGE_SCN_CNT_INITIALIZED_DATA  | \
> > +             IMAGE_SCN_MEM_READ              | \
> > +             IMAGE_SCN_MEM_DISCARDABLE       | \
> > +             IMAGE_SCN_ALIGN_1BYTES          # Characteristics
> >  #endif
> >
> >       #
> > @@ -263,7 +262,10 @@ section_table:
> >       .long   0                               # PointerToLineNumbers
> >       .word   0                               # NumberOfRelocations
> >       .word   0                               # NumberOfLineNumbers
> > -     .long   0x60500020                      # Characteristics (section flags)
> > +     .long   IMAGE_SCN_CNT_CODE              | \
> > +             IMAGE_SCN_MEM_READ              | \
> > +             IMAGE_SCN_MEM_EXECUTE           | \
> > +             IMAGE_SCN_ALIGN_16BYTES         # Characteristics
>
> The .text section contains data as well. Shouldn't it be marked
> IMAGE_SCN_MEM_WRITE also?
>

Another good point. For ARM and arm64, I actually changed this into
split R-X and RW- sections, in case the loader decides to honour these
attributes (which it should imo)

So yes, we should either add IMAGE_SCN_MEM_WRITE, or add a .data
section (although we probably don't have the space for that).

Another thing I wondered was whether we really need the .reloc
section. We don't have one on ARM, and it works fine with the existing
EDK2 loader.
Peter: any idea whether the issue with .reloc you pointed out to me
the other day still exists in EDK2 today?

Re: [RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Arvind Sankar]

On Thu, Feb 20, 2020 at 06:32:39PM +0100, Ard Biesheuvel wrote:
> Another thing I wondered was whether we really need the .reloc
> section. We don't have one on ARM, and it works fine with the existing
> EDK2 loader.
> Peter: any idea whether the issue with .reloc you pointed out to me
> the other day still exists in EDK2 today?

commit 743628e868c5 ("x86, efi stub: Add .reloc section back into
image") says that
	Some UEFI firmware will not load a .efi with a .reloc section
	with a size of 0.

Is that the issue you're refering to? It is a bit odd, since we actually
leave base relocation table at a size of zero with an RVA of zero, so it
shouldn't even look at the .reloc section according to the spec. At
least current EKD2 code doesn't seem to -- I think it would even work if
you specify fewer tables than 6 so that the base relocation table is
missing altogether.

Re: [RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Arvind Sankar]

On Thu, Feb 20, 2020 at 01:04:54PM -0500, Arvind Sankar wrote:
> On Thu, Feb 20, 2020 at 06:32:39PM +0100, Ard Biesheuvel wrote:
> > Another thing I wondered was whether we really need the .reloc
> > section. We don't have one on ARM, and it works fine with the existing
> > EDK2 loader.
> > Peter: any idea whether the issue with .reloc you pointed out to me
> > the other day still exists in EDK2 today?
> 
> commit 743628e868c5 ("x86, efi stub: Add .reloc section back into
> image") says that
> 	Some UEFI firmware will not load a .efi with a .reloc section
> 	with a size of 0.
> 
> Is that the issue you're refering to? It is a bit odd, since we actually
> leave base relocation table at a size of zero with an RVA of zero, so it
> shouldn't even look at the .reloc section according to the spec. At
> least current EKD2 code doesn't seem to -- I think it would even work if
> you specify fewer tables than 6 so that the base relocation table is
> missing altogether.

Another couple of odd things are that the PE header is supposed to be
8-byte aligned which we don't do, and NumberOfSymbols is set to 1 --
should be 0, no?

Re: [RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Ard Biesheuvel]

On Thu, 20 Feb 2020 at 19:05, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Thu, Feb 20, 2020 at 06:32:39PM +0100, Ard Biesheuvel wrote:
> > Another thing I wondered was whether we really need the .reloc
> > section. We don't have one on ARM, and it works fine with the existing
> > EDK2 loader.
> > Peter: any idea whether the issue with .reloc you pointed out to me
> > the other day still exists in EDK2 today?
>
> commit 743628e868c5 ("x86, efi stub: Add .reloc section back into
> image") says that
>         Some UEFI firmware will not load a .efi with a .reloc section
>         with a size of 0.
>
> Is that the issue you're refering to? It is a bit odd, since we actually
> leave base relocation table at a size of zero with an RVA of zero, so it
> shouldn't even look at the .reloc section according to the spec. At
> least current EKD2 code doesn't seem to -- I think it would even work if
> you specify fewer tables than 6 so that the base relocation table is
> missing altogether.

I can only agree with that, and I have never experienced any issues
loading PE/COFF images without .reloc sections on any architecture.

But looking at that patch, it seems it only changes the .reloc
section's size, it doesn't actually add it back. The .reloc section
has been there from the beginning, with a note saying that it is
required, which doesn't seem to be the case today, and looking at the
history of EDK2, I can't really spot any changes in that regard
either.

Re: [RFC PATCH 0/3] efi: put an API version number in the PE/COFF header

[Ard Biesheuvel]

On Thu, 20 Feb 2020 at 15:56, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 2/20/20 12:06 PM, Ard Biesheuvel wrote:
> > After having added new ways to load the initrd and the mixed mode kernel,
> > we will need to tell the loader about this, so it can use the new, generic
> > method if supported, and fall back to the existing method(s) otherwise.
> >
> > This is an RFC - if there are better ways to record this in the image
> > somehow, please shout.
>
>
> Hello Ard,
>
> for boot loaders like GRUB I understand that the boot loader could use
> the initrd file path from its scripts to prepare a
> EFI_LOAD_FILE2_PROTOCOL matching the loaded kernel.
>
> I am not sure about the requirements for a firmware.
>
> Up to now the U-Boot UEFI sub-system does not care about initial RAM
> disk images at all. With Ilias suggested patch series U-Boot could offer
> a file from a fixed file location via a EFI_LOAD_FILE2_PROTOCOL.
>
> Is there anything else you expect a firmware like U-Boot or EDK2 to do?
>

It highly depends on the use case. The distros really want GRUB, but
others really don't want GRUB (like Ilias). There are other use cases
we are considering where the initrd load is just a memcpy() from NOR
flash.

Given that the u-boot EFI subsystem was originally conceived to be
able to run GRUB, you might see them as complimentary, but one of the
things I am trying to do is ensure that GRUB is not essential for
booting, even taking things like UEFI secure boot and measure boot
into account. Currently, on x86, these are all tied together in a way
that is hard to extrapolate to other architectures.

Re: [RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Arvind Sankar]

On Thu, Feb 20, 2020 at 07:14:27PM +0100, Ard Biesheuvel wrote:
> On Thu, 20 Feb 2020 at 19:05, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Thu, Feb 20, 2020 at 06:32:39PM +0100, Ard Biesheuvel wrote:
> > > Another thing I wondered was whether we really need the .reloc
> > > section. We don't have one on ARM, and it works fine with the existing
> > > EDK2 loader.
> > > Peter: any idea whether the issue with .reloc you pointed out to me
> > > the other day still exists in EDK2 today?
> >
> > commit 743628e868c5 ("x86, efi stub: Add .reloc section back into
> > image") says that
> >         Some UEFI firmware will not load a .efi with a .reloc section
> >         with a size of 0.
> >
> > Is that the issue you're refering to? It is a bit odd, since we actually
> > leave base relocation table at a size of zero with an RVA of zero, so it
> > shouldn't even look at the .reloc section according to the spec. At
> > least current EKD2 code doesn't seem to -- I think it would even work if
> > you specify fewer tables than 6 so that the base relocation table is
> > missing altogether.
> 
> I can only agree with that, and I have never experienced any issues
> loading PE/COFF images without .reloc sections on any architecture.
> 
> But looking at that patch, it seems it only changes the .reloc
> section's size, it doesn't actually add it back. The .reloc section
> has been there from the beginning, with a note saying that it is
> required, which doesn't seem to be the case today, and looking at the
> history of EDK2, I can't really spot any changes in that regard
> either.

Yeah I thought given that it caused a problem some firmware must have
been looking at the reloc section, but on the other hand it might just
have been that the firmware barfed because it didn't expect any section
to be zero size?

Re: [RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Arvind Sankar]

On Thu, Feb 20, 2020 at 06:32:39PM +0100, Ard Biesheuvel wrote:
> On Thu, 20 Feb 2020 at 18:28, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > The .text section contains data as well. Shouldn't it be marked
> > IMAGE_SCN_MEM_WRITE also?
> >
> 
> Another good point. For ARM and arm64, I actually changed this into
> split R-X and RW- sections, in case the loader decides to honour these
> attributes (which it should imo)
> 
> So yes, we should either add IMAGE_SCN_MEM_WRITE, or add a .data
> section (although we probably don't have the space for that).
> 
> Another thing I wondered was whether we really need the .reloc
> section. We don't have one on ARM, and it works fine with the existing
> EDK2 loader.
> Peter: any idea whether the issue with .reloc you pointed out to me
> the other day still exists in EDK2 today?

Is there any reason we couldn't combine .setup + .text as R-X, then
.reloc, then .data + .bss as RW-? That would leave room for the new
.compat section then even without removing .reloc.

Although if the loader actually honored the non-writable setting,
efi_relocate_kernel has to be done unconditionally. It currently happens
to be unconditional if you come in via PE loader, since we don't set
ImageBase to tell it our preferred address, and even if we did, the
existence of .setup means startup32 isn't at that address.

Re: [RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Ard Biesheuvel]

On Thu, 20 Feb 2020 at 20:52, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Thu, Feb 20, 2020 at 06:32:39PM +0100, Ard Biesheuvel wrote:
> > On Thu, 20 Feb 2020 at 18:28, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > The .text section contains data as well. Shouldn't it be marked
> > > IMAGE_SCN_MEM_WRITE also?
> > >
> >
> > Another good point. For ARM and arm64, I actually changed this into
> > split R-X and RW- sections, in case the loader decides to honour these
> > attributes (which it should imo)
> >
> > So yes, we should either add IMAGE_SCN_MEM_WRITE, or add a .data
> > section (although we probably don't have the space for that).
> >
> > Another thing I wondered was whether we really need the .reloc
> > section. We don't have one on ARM, and it works fine with the existing
> > EDK2 loader.
> > Peter: any idea whether the issue with .reloc you pointed out to me
> > the other day still exists in EDK2 today?
>
> Is there any reason we couldn't combine .setup + .text as R-X, then
> .reloc, then .data + .bss as RW-? That would leave room for the new
> .compat section then even without removing .reloc.
>
> Although if the loader actually honored the non-writable setting,
> efi_relocate_kernel has to be done unconditionally. It currently happens
> to be unconditional if you come in via PE loader, since we don't set
> ImageBase to tell it our preferred address, and even if we did, the
> existence of .setup means startup32 isn't at that address.

Shouldn't matter, given that we don't use the same page tables to run
the startup code.

Re: [RFC PATCH 1/3] efi/x86: Use symbolic constants in PE header instead of bare numbers

[Arvind Sankar]

On Thu, Feb 20, 2020 at 10:22:23PM +0100, Ard Biesheuvel wrote:
> On Thu, 20 Feb 2020 at 20:52, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Thu, Feb 20, 2020 at 06:32:39PM +0100, Ard Biesheuvel wrote:
> > > On Thu, 20 Feb 2020 at 18:28, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > The .text section contains data as well. Shouldn't it be marked
> > > > IMAGE_SCN_MEM_WRITE also?
> > > >
> > >
> > > Another good point. For ARM and arm64, I actually changed this into
> > > split R-X and RW- sections, in case the loader decides to honour these
> > > attributes (which it should imo)
> > >
> > > So yes, we should either add IMAGE_SCN_MEM_WRITE, or add a .data
> > > section (although we probably don't have the space for that).
> > >
> > > Another thing I wondered was whether we really need the .reloc
> > > section. We don't have one on ARM, and it works fine with the existing
> > > EDK2 loader.
> > > Peter: any idea whether the issue with .reloc you pointed out to me
> > > the other day still exists in EDK2 today?
> >
> > Is there any reason we couldn't combine .setup + .text as R-X, then
> > .reloc, then .data + .bss as RW-? That would leave room for the new
> > .compat section then even without removing .reloc.
> >
> > Although if the loader actually honored the non-writable setting,
> > efi_relocate_kernel has to be done unconditionally. It currently happens
> > to be unconditional if you come in via PE loader, since we don't set
> > ImageBase to tell it our preferred address, and even if we did, the
> > existence of .setup means startup32 isn't at that address.
> 
> Shouldn't matter, given that we don't use the same page tables to run
> the startup code.

Ooh right. And for the 32-bit case we turn off paging altogether.