[PATCH v2 0/8] linux: implement LoadFile2 initrd loading

[PATCH v2 0/8] linux: implement LoadFile2 initrd loading

[Ard Biesheuvel]

This implements the LoadFile2 initrd loading protocol, which is
essentially a callback face into the bootloader to load the initrd
data into a caller provided buffer. This means the bootloader no
longer has to contain any policy regarding where to load the initrd
(which differs between architectures and kernel versions) and no
longer has to manipulate arch specific data structures such as DT
or struct bootparams to inform the OS where the initrd resides in
memory.

Sample output from booting a recent Linux/arm64 kernel:

  grub> insmod part_msdos
  grub> linux (hd0,msdos1)/Image
  grub> initrd (hd0,msdos1)/initrd.img
  grub> boot
  EFI stub: Booting Linux Kernel...
  EFI stub: EFI_RNG_PROTOCOL unavailable, KASLR will be disabled
  EFI stub: Generating empty DTB
  EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
  EFI stub: Exiting boot services and installing virtual address map...
  [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x411fd070]

Cc: grub-devel@gnu.org
Cc: daniel.kiper@oracle.com
Cc: leif@nuviainc.com

Changes since v1:
- add Leif's ack to #2, #6 and #8
- add patch #1 to fix the ARM Linux image header definition
- add patches #3, #4 and #5 to unify and reuse the COFF header loading routines
  from Xen (and the absence of which from v1 was causing garbage to be read and
  interpreted as the image major version)

Ard Biesheuvel (8):
  linux/arm: fix ARM Linux header layout
  loader/linux: permit NULL argument for argv[] in grub_initrd_load()
  efi: move MS-DOS stub out of generic PE header definition
  linux/arm: unify ARM/arm64 vs Xen PE/COFF header handling
  linux/arm: account for COFF headers appearing at unexpected offsets
  efi: add definition of LoadFile2 protocol
  efi: implement LoadFile2 initrd loading protocol for Linux
  linux: ignore FDT unless we need to modify it

 grub-core/commands/efi/lsefi.c    |   1 +
 grub-core/kern/efi/efi.c          |   5 +-
 grub-core/loader/arm64/linux.c    | 158 +++++++++++++++++---
 grub-core/loader/arm64/xen_boot.c |  23 +--
 grub-core/loader/efi/fdt.c        |   7 +-
 grub-core/loader/linux.c          |   2 +-
 include/grub/arm/linux.h          |   8 +-
 include/grub/arm64/linux.h        |   4 +
 include/grub/efi/api.h            |  15 ++
 include/grub/efi/efi.h            |   4 +-
 include/grub/efi/pe32.h           |   5 +-
 11 files changed, 185 insertions(+), 47 deletions(-)

-- 
2.17.1

[PATCH v2 1/8] linux/arm: fix ARM Linux header layout

[Ard Biesheuvel]

The hdr_offset member of the ARM Linux image header appears at
offset 0x3c, matching the PE/COFF spec's placement of the COFF
header offset in the MS-DOS header. We're currently off by four,
so fix that.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 include/grub/arm/linux.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
index 2e98a6689696..bcd5a7eb186e 100644
--- a/include/grub/arm/linux.h
+++ b/include/grub/arm/linux.h
@@ -30,7 +30,7 @@ struct linux_arm_kernel_header {
   grub_uint32_t magic;
   grub_uint32_t start; /* _start */
   grub_uint32_t end;   /* _edata */
-  grub_uint32_t reserved2[4];
+  grub_uint32_t reserved2[3];
   grub_uint32_t hdr_offset;
 };
 
-- 
2.17.1

[PATCH v2 2/8] loader/linux: permit NULL argument for argv[] in grub_initrd_load()

[Ard Biesheuvel]

grub_initrd_load() takes a char *argv[] argument which is only used
when an error occurs, to print the name of the file that caused the
error. In order to be able to split initrd loading from handling the
initrd command, let's permit argv to be NULL, and fall back to the
file names recorded in the file handles.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
---
 grub-core/loader/linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/loader/linux.c b/grub-core/loader/linux.c
index 3fe390f17ec6..8c01381778c6 100644
--- a/grub-core/loader/linux.c
+++ b/grub-core/loader/linux.c
@@ -317,7 +317,7 @@ grub_initrd_load (struct grub_linux_initrd_context *initrd_ctx,
 	{
 	  if (!grub_errno)
 	    grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file %s"),
-			argv[i]);
+			argv ? argv[i] : initrd_ctx->components[i].file->name);
 	  grub_initrd_close (initrd_ctx);
 	  return grub_errno;
 	}
-- 
2.17.1

[PATCH v2 3/8] efi: move MS-DOS stub out of generic PE header definition

[Ard Biesheuvel]

The PE/COFF spec permits the COFF signature and file header to appear
anywhere in the file, and the actual offset is recorded in 4 byte
little endian field at offset 0x3c of the image.

When GRUB is emitted as a PE/COFF binary, we reuse the 128 byte MS-DOS
stub (even for non-x86 architectures), putting the COFF signature and
file header at offset 0x80. However, other PE/COFF images may use
different values, and non-x86 Linux kernels use an offset of 0x40
instead.

So let's get rid of the grub_pe32_header struct from pe32.h, given that
it does not represent anything defined by the PE/COFF spec. Instead,
use the GRUB_PE32_MSDOS_STUB_SIZE macro explicitly to reference the
COFF header in the only place in the code where we rely on this.

The remaining fields are moved into a struct grub_coff_image_header,
which we will use later to access COFF header fields of arbitrary
images (and which may therefore appear at different offsets)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 grub-core/kern/efi/efi.c | 5 +++--
 include/grub/efi/pe32.h  | 5 +----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index e0165e74c587..9e5a72be538d 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -282,7 +282,7 @@ grub_addr_t
 grub_efi_modules_addr (void)
 {
   grub_efi_loaded_image_t *image;
-  struct grub_pe32_header *header;
+  struct grub_coff_image_header *header;
   struct grub_pe32_coff_header *coff_header;
   struct grub_pe32_section_table *sections;
   struct grub_pe32_section_table *section;
@@ -293,7 +293,8 @@ grub_efi_modules_addr (void)
   if (! image)
     return 0;
 
-  header = image->image_base;
+  header = (struct grub_coff_image_header *) ((char *) image->image_base
+					      + GRUB_PE32_MSDOS_STUB_SIZE);
   coff_header = &(header->coff_header);
   sections
     = (struct grub_pe32_section_table *) ((char *) coff_header
diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
index 0ed8781f0376..a2da4b318c85 100644
--- a/include/grub/efi/pe32.h
+++ b/include/grub/efi/pe32.h
@@ -254,11 +254,8 @@ struct grub_pe32_section_table
 
 #define GRUB_PE32_SIGNATURE_SIZE 4
 
-struct grub_pe32_header
+struct grub_coff_image_header
 {
-  /* This should be filled in with GRUB_PE32_MSDOS_STUB.  */
-  grub_uint8_t msdos_stub[GRUB_PE32_MSDOS_STUB_SIZE];
-
   /* This is always PE\0\0.  */
   char signature[GRUB_PE32_SIGNATURE_SIZE];
 
-- 
2.17.1

[PATCH v2 4/8] linux/arm: unify ARM/arm64 vs Xen PE/COFF header handling

[Ard Biesheuvel]

Xen has its own version of the image header, to account for the
additional PE/COFF header fields. Since we are adding references to
those in the shared EFI loader code, update the common definitions
and drop the Xen specific one which no longer has a purpose.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 grub-core/loader/arm64/linux.c    | 12 +++++-----
 grub-core/loader/arm64/xen_boot.c | 23 ++++----------------
 include/grub/arm/linux.h          |  6 +++++
 include/grub/arm64/linux.h        |  4 ++++
 include/grub/efi/efi.h            |  4 +++-
 5 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index ef3e9f9444ca..915b6ad7292d 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -49,8 +49,13 @@ static grub_addr_t initrd_start;
 static grub_addr_t initrd_end;
 
 grub_err_t
-grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh)
+grub_arch_efi_linux_load_image_header (grub_file_t file,
+				       struct linux_arch_kernel_header * lh)
 {
+  grub_file_seek (file, 0);
+  if (grub_file_read (file, lh, sizeof (*lh)) < (long) sizeof (*lh))
+    return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read Linux image header");
+
   if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE)
     return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
 
@@ -304,10 +309,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 
   kernel_size = grub_file_size (file);
 
-  if (grub_file_read (file, &lh, sizeof (lh)) < (long) sizeof (lh))
-    return grub_errno;
-
-  if (grub_arch_efi_linux_check_image (&lh) != GRUB_ERR_NONE)
+  if (grub_arch_efi_linux_load_image_header (file, &lh) != GRUB_ERR_NONE)
     goto fail;
 
   grub_loader_unset();
diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c
index 22cc25eccd9d..e5895ee78218 100644
--- a/grub-core/loader/arm64/xen_boot.c
+++ b/grub-core/loader/arm64/xen_boot.c
@@ -31,7 +31,6 @@
 #include <grub/efi/efi.h>
 #include <grub/efi/fdtload.h>
 #include <grub/efi/memory.h>
-#include <grub/efi/pe32.h>	/* required by struct xen_hypervisor_header */
 #include <grub/i18n.h>
 #include <grub/lib/cmdline.h>
 
@@ -65,18 +64,6 @@ enum module_type
 };
 typedef enum module_type module_type_t;
 
-struct xen_hypervisor_header
-{
-  struct linux_arm64_kernel_header efi_head;
-
-  /* This is always PE\0\0.  */
-  grub_uint8_t signature[GRUB_PE32_SIGNATURE_SIZE];
-  /* The COFF file header.  */
-  struct grub_pe32_coff_header coff_header;
-  /* The Optional header.  */
-  struct grub_pe64_optional_header optional_header;
-};
-
 struct xen_boot_binary
 {
   struct xen_boot_binary *next;
@@ -452,7 +439,7 @@ static grub_err_t
 grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)),
 			 int argc, char *argv[])
 {
-  struct xen_hypervisor_header sh;
+  struct linux_arm64_kernel_header lh;
   grub_file_t file = NULL;
 
   grub_dl_ref (my_mod);
@@ -467,10 +454,7 @@ grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)),
   if (!file)
     goto fail;
 
-  if (grub_file_read (file, &sh, sizeof (sh)) != (long) sizeof (sh))
-    goto fail;
-  if (grub_arch_efi_linux_check_image
-      ((struct linux_arch_kernel_header *) &sh) != GRUB_ERR_NONE)
+  if (grub_arch_efi_linux_load_image_header (file, &lh) != GRUB_ERR_NONE)
     goto fail;
   grub_file_seek (file, 0);
 
@@ -484,7 +468,8 @@ grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)),
     return grub_errno;
 
   xen_hypervisor->is_hypervisor = 1;
-  xen_hypervisor->align = (grub_size_t) sh.optional_header.section_alignment;
+  xen_hypervisor->align
+    = (grub_size_t) lh.coff_image_header.optional_header.section_alignment;
 
   xen_boot_binary_load (xen_hypervisor, file, argc, argv);
   if (grub_errno == GRUB_ERR_NONE)
diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
index bcd5a7eb186e..ea815db13417 100644
--- a/include/grub/arm/linux.h
+++ b/include/grub/arm/linux.h
@@ -22,6 +22,8 @@
 
 #include "system.h"
 
+#include <grub/efi/pe32.h>
+
 #define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
 
 struct linux_arm_kernel_header {
@@ -32,6 +34,10 @@ struct linux_arm_kernel_header {
   grub_uint32_t end;   /* _edata */
   grub_uint32_t reserved2[3];
   grub_uint32_t hdr_offset;
+
+#if defined GRUB_MACHINE_EFI
+  struct grub_coff_image_header coff_image_header;
+#endif
 };
 
 #if defined(__arm__)
diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
index 4269adc6dae5..e5a7ed3749d1 100644
--- a/include/grub/arm64/linux.h
+++ b/include/grub/arm64/linux.h
@@ -19,6 +19,8 @@
 #ifndef GRUB_ARM64_LINUX_HEADER
 #define GRUB_ARM64_LINUX_HEADER 1
 
+#include <grub/efi/pe32.h>
+
 #define GRUB_LINUX_ARM64_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
 
 /* From linux/Documentation/arm64/booting.txt */
@@ -34,6 +36,8 @@ struct linux_arm64_kernel_header
   grub_uint64_t res4;		/* reserved */
   grub_uint32_t magic;		/* Magic number, little endian, "ARM\x64" */
   grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
+
+  struct grub_coff_image_header	coff_image_header;
 };
 
 #if defined(__aarch64__)
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index e90e00dc431b..93cb2f5bc732 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -94,7 +94,9 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
 void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
 grub_err_t EXPORT_FUNC(grub_efi_get_ram_base)(grub_addr_t *);
 #include <grub/cpu/linux.h>
-grub_err_t grub_arch_efi_linux_check_image(struct linux_arch_kernel_header *lh);
+#include <grub/file.h>
+grub_err_t grub_arch_efi_linux_load_image_header(grub_file_t file,
+					         struct linux_arch_kernel_header *lh);
 grub_err_t grub_arch_efi_linux_boot_image(grub_addr_t addr, grub_size_t size,
                                            char *args);
 #endif
-- 
2.17.1

[PATCH v2 5/8] linux/arm: account for COFF headers appearing at unexpected offsets

[Ard Biesheuvel]

The way we load the Linux and PE/COFF image headers depends on a fixed
placement of the COFF header at offset 0x40 into the file. This is a
reasonable default, given that this is where Linux emits it today.
However, in order to comply with the PE/COFF spec, which permits this
header to appear anywhere in the file, let's ensure that we read the
header from where it actually appears in the file if it is not located
at offset 0x40.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 grub-core/loader/arm64/linux.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index 915b6ad7292d..28ff8584a3b5 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
   grub_dprintf ("linux", "UEFI stub kernel:\n");
   grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
 
+  /*
+   * The PE/COFF spec permits the COFF header to appear anywhere in the file, so
+   * we need to double check whether it was where we expected it, and if not, we
+   * must load it from the correct offset into the coff_image_header field of
+   * struct linux_arch_kernel_header.
+   */
+  if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header)
+    {
+      grub_file_seek (file, lh->hdr_offset);
+
+      if (grub_file_read (file, &lh->coff_image_header, sizeof(struct grub_coff_image_header))
+	  != sizeof(struct grub_coff_image_header))
+	return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF image header");
+    }
+
   return GRUB_ERR_NONE;
 }
 
-- 
2.17.1

[PATCH v2 6/8] efi: add definition of LoadFile2 protocol

[Ard Biesheuvel]

Incorporate the EFI_LOAD_FILE2_PROTOCOL GUID and C types from the
UEFI spec.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
---
 grub-core/commands/efi/lsefi.c |  1 +
 include/grub/efi/api.h         | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c
index d1ce99af4389..145cbfcdaf5c 100644
--- a/grub-core/commands/efi/lsefi.c
+++ b/grub-core/commands/efi/lsefi.c
@@ -55,6 +55,7 @@ struct known_protocol
     { GRUB_EFI_ABSOLUTE_POINTER_PROTOCOL_GUID, "absolute pointer" },
     { GRUB_EFI_DRIVER_BINDING_PROTOCOL_GUID, "EFI driver binding" },
     { GRUB_EFI_LOAD_FILE_PROTOCOL_GUID, "load file" },
+    { GRUB_EFI_LOAD_FILE2_PROTOCOL_GUID, "load file 2" },
     { GRUB_EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID, "simple FS" },
     { GRUB_EFI_TAPE_IO_PROTOCOL_GUID, "tape I/O" },
     { GRUB_EFI_UNICODE_COLLATION_PROTOCOL_GUID, "unicode collation" },
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 1dcaa12f59e8..8bc040d9251b 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -149,6 +149,11 @@
     { 0x8E, 0x3F, 0x00, 0xA0, 0xC9, 0x69, 0x72, 0x3B } \
   }
 
+#define GRUB_EFI_LOAD_FILE2_PROTOCOL_GUID \
+  { 0x4006c0c1, 0xfcb3, 0x403e, \
+    { 0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d } \
+  }
+
 #define GRUB_EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID \
   { 0x0964e5b22, 0x6459, 0x11d2, \
     { 0x8e, 0x39, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b } \
@@ -1694,6 +1699,16 @@ struct grub_efi_block_io
 };
 typedef struct grub_efi_block_io grub_efi_block_io_t;
 
+struct grub_efi_load_file2
+{
+  grub_efi_status_t (*load_file)(struct grub_efi_load_file2 *this,
+				 grub_efi_device_path_t *file_path,
+				 grub_efi_boolean_t boot_policy,
+				 grub_efi_uintn_t *buffer_size,
+				 void *buffer);
+};
+typedef struct grub_efi_load_file2 grub_efi_load_file2_t;
+
 #if (GRUB_TARGET_SIZEOF_VOID_P == 4) || defined (__ia64__) \
   || defined (__aarch64__) || defined (__MINGW64__) || defined (__CYGWIN__) \
   || defined(__riscv)
-- 
2.17.1

[PATCH v2 7/8] efi: implement LoadFile2 initrd loading protocol for Linux

[Ard Biesheuvel]

Recent Linux kernels will invoke the LoadFile2 protocol installed on a
well-known vendor media devicepath to load the initrd if it is exposed
by the firmware. Using this method is preferred for two reasons:
- the Linux kernel is in charge of allocating the memory, and so it can
  implement any placement policy it wants (given that these tend to
  change between kernel versions),
- it is no longer necessary to modify the device tree provided by the
  firmware.

So let's install this protocol when handling the 'initrd' command if
such a recent kernel was detected (based on the PE/COFF image version),
and defer loading the initrd contents until the point where the kernel
invokes the LoadFile2 protocol.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 grub-core/loader/arm64/linux.c | 109 +++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index 28ff8584a3b5..c6a95e1f0c43 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -48,6 +48,10 @@ static grub_uint32_t cmdline_size;
 static grub_addr_t initrd_start;
 static grub_addr_t initrd_end;
 
+static struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
+static grub_efi_handle_t initrd_lf2_handle;
+static int initrd_use_loadfile2;
+
 grub_err_t
 grub_arch_efi_linux_load_image_header (grub_file_t file,
 				       struct linux_arch_kernel_header * lh)
@@ -81,6 +85,18 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
 	return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF image header");
     }
 
+  /*
+   * Linux kernels built for any architecture are guaranteed to support the
+   * LoadFile2 based initrd loading protocol if the image version is >= 1.
+   */
+  if (lh->coff_image_header.optional_header.major_image_version >= 1)
+    initrd_use_loadfile2 = 1;
+  else
+    initrd_use_loadfile2 = 0;
+
+  grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n",
+		initrd_use_loadfile2 ? "en" : "dis");
+
   return GRUB_ERR_NONE;
 }
 
@@ -250,13 +266,86 @@ allocate_initrd_mem (int initrd_pages)
 				       GRUB_EFI_LOADER_DATA);
 }
 
+struct initrd_media_device_path {
+  grub_efi_vendor_media_device_path_t	vendor;
+  grub_efi_device_path_t		end;
+} GRUB_PACKED;
+
+#define LINUX_EFI_INITRD_MEDIA_GUID  \
+  { 0x5568e427, 0x68fc, 0x4f3d, \
+    { 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 } \
+  }
+
+static struct initrd_media_device_path initrd_lf2_device_path = {
+  {
+    {
+      GRUB_EFI_MEDIA_DEVICE_PATH_TYPE,
+      GRUB_EFI_VENDOR_MEDIA_DEVICE_PATH_SUBTYPE,
+      sizeof(grub_efi_vendor_media_device_path_t),
+    },
+    LINUX_EFI_INITRD_MEDIA_GUID
+  }, {
+    GRUB_EFI_END_DEVICE_PATH_TYPE,
+    GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE,
+    sizeof(grub_efi_device_path_t)
+  }
+};
+
+static grub_efi_status_t initrd_load_file2(grub_efi_load_file2_t *this,
+					   grub_efi_device_path_t *device_path,
+					   grub_efi_boolean_t boot_policy,
+					   grub_efi_uintn_t *buffer_size,
+					   void *buffer);
+
+static grub_efi_load_file2_t initrd_lf2 = {
+  initrd_load_file2
+};
+
+static grub_efi_status_t initrd_load_file2(grub_efi_load_file2_t *this,
+					   grub_efi_device_path_t *device_path,
+					   grub_efi_boolean_t boot_policy,
+					   grub_efi_uintn_t *buffer_size,
+					   void *buffer)
+{
+  grub_efi_status_t status = GRUB_EFI_SUCCESS;
+  grub_efi_uintn_t initrd_size;
+
+  if (!this || this != &initrd_lf2 || !buffer_size)
+    return GRUB_EFI_INVALID_PARAMETER;
+
+  if (device_path->type != GRUB_EFI_END_DEVICE_PATH_TYPE ||
+      device_path->subtype != GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE)
+    return GRUB_EFI_NOT_FOUND;
+
+  if (boot_policy)
+    return GRUB_EFI_UNSUPPORTED;
+
+  initrd_size = grub_get_initrd_size (&initrd_ctx);
+  if (!buffer || *buffer_size < initrd_size)
+    {
+      *buffer_size = initrd_size;
+      return GRUB_EFI_BUFFER_TOO_SMALL;
+    }
+
+  grub_dprintf ("linux", "Loading initrd via LOAD_FILE2_PROTOCOL\n");
+
+  if (grub_initrd_load (&initrd_ctx, NULL, buffer))
+    status = GRUB_EFI_LOAD_ERROR;
+
+  grub_initrd_close (&initrd_ctx);
+  return status;
+}
+
 static grub_err_t
 grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
 		 int argc, char *argv[])
 {
-  struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
   int initrd_size, initrd_pages;
   void *initrd_mem = NULL;
+  grub_efi_guid_t load_file2_guid = GRUB_EFI_LOAD_FILE2_PROTOCOL_GUID;
+  grub_efi_guid_t device_path_guid = GRUB_EFI_DEVICE_PATH_GUID;
+  grub_efi_boot_services_t *b;
+  grub_efi_status_t status;
 
   if (argc == 0)
     {
@@ -274,6 +363,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
   if (grub_initrd_init (argc, argv, &initrd_ctx))
     goto fail;
 
+  if (initrd_use_loadfile2)
+    {
+      b = grub_efi_system_table->boot_services;
+      status = b->install_multiple_protocol_interfaces (&initrd_lf2_handle,
+							&load_file2_guid,
+							&initrd_lf2,
+							&device_path_guid,
+							&initrd_lf2_device_path,
+							NULL);
+      if (status == GRUB_EFI_OUT_OF_RESOURCES)
+	{
+	  grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
+	  return grub_errno;
+	}
+      grub_dprintf ("linux", "LoadFile2 initrd loading protocol installed\n");
+      return GRUB_ERR_NONE;
+    }
+
   initrd_size = grub_get_initrd_size (&initrd_ctx);
   grub_dprintf ("linux", "Loading initrd\n");
 
-- 
2.17.1

[PATCH v2 8/8] linux: ignore FDT unless we need to modify it

[Ard Biesheuvel]

Now that we implemented support for the LoadFile2 protocol for initrd
loading, there is no longer a need to pass the initrd parameters via
the device tree. This means there is no longer a reason to update the
device tree in the first place, and so we can ignore it entirely.

The only remaining reason to deal with the devicetree is if we are
using the 'devicetree' command to load one from disk, so tweak the
logic in grub_fdt_install() to take that into account.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
---
 grub-core/loader/arm64/linux.c | 22 ++++++++++----------
 grub-core/loader/efi/fdt.c     |  7 +++++--
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index c6a95e1f0c43..248277c753b6 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -107,21 +107,21 @@ finalize_params_linux (void)
 
   void *fdt;
 
-  fdt = grub_fdt_load (GRUB_EFI_LINUX_FDT_EXTRA_SPACE);
+  /* Set initrd info */
+  if (initrd_start && initrd_end > initrd_start)
+    {
+      fdt = grub_fdt_load (GRUB_EFI_LINUX_FDT_EXTRA_SPACE);
 
-  if (!fdt)
-    goto failure;
+      if (!fdt)
+	goto failure;
 
-  node = grub_fdt_find_subnode (fdt, 0, "chosen");
-  if (node < 0)
-    node = grub_fdt_add_subnode (fdt, 0, "chosen");
+      node = grub_fdt_find_subnode (fdt, 0, "chosen");
+      if (node < 0)
+	node = grub_fdt_add_subnode (fdt, 0, "chosen");
 
-  if (node < 1)
-    goto failure;
+      if (node < 1)
+	goto failure;
 
-  /* Set initrd info */
-  if (initrd_start && initrd_end > initrd_start)
-    {
       grub_dprintf ("linux", "Initrd @ %p-%p\n",
 		    (void *) initrd_start, (void *) initrd_end);
 
diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
index ee9c5592c700..ab900b27d927 100644
--- a/grub-core/loader/efi/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -85,13 +85,16 @@ grub_fdt_install (void)
   grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
   grub_efi_status_t status;
 
+  if (!fdt && !loaded_fdt)
+    return GRUB_ERR_NONE;
+
   b = grub_efi_system_table->boot_services;
-  status = b->install_configuration_table (&fdt_guid, fdt);
+  status = b->install_configuration_table (&fdt_guid, fdt ?: loaded_fdt);
   if (status != GRUB_EFI_SUCCESS)
     return grub_error (GRUB_ERR_IO, "failed to install FDT");
 
   grub_dprintf ("fdt", "Installed/updated FDT configuration table @ %p\n",
-		fdt);
+		fdt ?: loaded_fdt);
   return GRUB_ERR_NONE;
 }
 
-- 
2.17.1

Re: [PATCH v2 7/8] efi: implement LoadFile2 initrd loading protocol for Linux

[Atish Patra]

On Sun, Oct 25, 2020 at 6:50 AM Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:
>
> Recent Linux kernels will invoke the LoadFile2 protocol installed on a
> well-known vendor media devicepath to load the initrd if it is exposed
> by the firmware. Using this method is preferred for two reasons:
> - the Linux kernel is in charge of allocating the memory, and so it can
>   implement any placement policy it wants (given that these tend to
>   change between kernel versions),
> - it is no longer necessary to modify the device tree provided by the
>   firmware.
>
> So let's install this protocol when handling the 'initrd' command if
> such a recent kernel was detected (based on the PE/COFF image version),
> and defer loading the initrd contents until the point where the kernel
> invokes the LoadFile2 protocol.
>

Thanks for adding the support for LoadFile2 protocol. This was one of
the blockers for full RISC-V support.
How do you prefer to proceed with RISC-V support ?

For RISC-V, we just need to move the arm64 loader to a common file so
that both RISC-V/ARM64 can use it apart from
few header file fixes. During the last attempt[1], I moved it to
loader/efi/linux.c.

I am happy to test it on Qemu/hardware, if you want to send the series
either part of this one or a separate series.
I can rebase my previous series on top of this series as well. Please
let me know your preference.

[1] https://www.mail-archive.com/grub-devel@gnu.org/msg30107.html

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  grub-core/loader/arm64/linux.c | 109 +++++++++++++++++++-
>  1 file changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 28ff8584a3b5..c6a95e1f0c43 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -48,6 +48,10 @@ static grub_uint32_t cmdline_size;
>  static grub_addr_t initrd_start;
>  static grub_addr_t initrd_end;
>
> +static struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
> +static grub_efi_handle_t initrd_lf2_handle;
> +static int initrd_use_loadfile2;
> +
>  grub_err_t
>  grub_arch_efi_linux_load_image_header (grub_file_t file,
>                                        struct linux_arch_kernel_header * lh)
> @@ -81,6 +85,18 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
>         return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF image header");
>      }
>
> +  /*
> +   * Linux kernels built for any architecture are guaranteed to support the
> +   * LoadFile2 based initrd loading protocol if the image version is >= 1.
> +   */
> +  if (lh->coff_image_header.optional_header.major_image_version >= 1)
> +    initrd_use_loadfile2 = 1;
> +  else
> +    initrd_use_loadfile2 = 0;
> +
> +  grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n",
> +               initrd_use_loadfile2 ? "en" : "dis");
> +
>    return GRUB_ERR_NONE;
>  }
>
> @@ -250,13 +266,86 @@ allocate_initrd_mem (int initrd_pages)
>                                        GRUB_EFI_LOADER_DATA);
>  }
>
> +struct initrd_media_device_path {
> +  grub_efi_vendor_media_device_path_t  vendor;
> +  grub_efi_device_path_t               end;
> +} GRUB_PACKED;
> +
> +#define LINUX_EFI_INITRD_MEDIA_GUID  \
> +  { 0x5568e427, 0x68fc, 0x4f3d, \
> +    { 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 } \
> +  }
> +
> +static struct initrd_media_device_path initrd_lf2_device_path = {
> +  {
> +    {
> +      GRUB_EFI_MEDIA_DEVICE_PATH_TYPE,
> +      GRUB_EFI_VENDOR_MEDIA_DEVICE_PATH_SUBTYPE,
> +      sizeof(grub_efi_vendor_media_device_path_t),
> +    },
> +    LINUX_EFI_INITRD_MEDIA_GUID
> +  }, {
> +    GRUB_EFI_END_DEVICE_PATH_TYPE,
> +    GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +    sizeof(grub_efi_device_path_t)
> +  }
> +};
> +
> +static grub_efi_status_t initrd_load_file2(grub_efi_load_file2_t *this,
> +                                          grub_efi_device_path_t *device_path,
> +                                          grub_efi_boolean_t boot_policy,
> +                                          grub_efi_uintn_t *buffer_size,
> +                                          void *buffer);
> +
> +static grub_efi_load_file2_t initrd_lf2 = {
> +  initrd_load_file2
> +};
> +
> +static grub_efi_status_t initrd_load_file2(grub_efi_load_file2_t *this,
> +                                          grub_efi_device_path_t *device_path,
> +                                          grub_efi_boolean_t boot_policy,
> +                                          grub_efi_uintn_t *buffer_size,
> +                                          void *buffer)
> +{
> +  grub_efi_status_t status = GRUB_EFI_SUCCESS;
> +  grub_efi_uintn_t initrd_size;
> +
> +  if (!this || this != &initrd_lf2 || !buffer_size)
> +    return GRUB_EFI_INVALID_PARAMETER;
> +
> +  if (device_path->type != GRUB_EFI_END_DEVICE_PATH_TYPE ||
> +      device_path->subtype != GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE)
> +    return GRUB_EFI_NOT_FOUND;
> +
> +  if (boot_policy)
> +    return GRUB_EFI_UNSUPPORTED;
> +
> +  initrd_size = grub_get_initrd_size (&initrd_ctx);
> +  if (!buffer || *buffer_size < initrd_size)
> +    {
> +      *buffer_size = initrd_size;
> +      return GRUB_EFI_BUFFER_TOO_SMALL;
> +    }
> +
> +  grub_dprintf ("linux", "Loading initrd via LOAD_FILE2_PROTOCOL\n");
> +
> +  if (grub_initrd_load (&initrd_ctx, NULL, buffer))
> +    status = GRUB_EFI_LOAD_ERROR;
> +
> +  grub_initrd_close (&initrd_ctx);
> +  return status;
> +}
> +
>  static grub_err_t
>  grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
>                  int argc, char *argv[])
>  {
> -  struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
>    int initrd_size, initrd_pages;
>    void *initrd_mem = NULL;
> +  grub_efi_guid_t load_file2_guid = GRUB_EFI_LOAD_FILE2_PROTOCOL_GUID;
> +  grub_efi_guid_t device_path_guid = GRUB_EFI_DEVICE_PATH_GUID;
> +  grub_efi_boot_services_t *b;
> +  grub_efi_status_t status;
>
>    if (argc == 0)
>      {
> @@ -274,6 +363,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
>    if (grub_initrd_init (argc, argv, &initrd_ctx))
>      goto fail;
>
> +  if (initrd_use_loadfile2)
> +    {
> +      b = grub_efi_system_table->boot_services;
> +      status = b->install_multiple_protocol_interfaces (&initrd_lf2_handle,
> +                                                       &load_file2_guid,
> +                                                       &initrd_lf2,
> +                                                       &device_path_guid,
> +                                                       &initrd_lf2_device_path,
> +                                                       NULL);
> +      if (status == GRUB_EFI_OUT_OF_RESOURCES)
> +       {
> +         grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> +         return grub_errno;
> +       }
> +      grub_dprintf ("linux", "LoadFile2 initrd loading protocol installed\n");
> +      return GRUB_ERR_NONE;
> +    }
> +
>    initrd_size = grub_get_initrd_size (&initrd_ctx);
>    grub_dprintf ("linux", "Loading initrd\n");
>
> --
> 2.17.1
>


-- 
Regards,
Atish

Re: [PATCH v2 7/8] efi: implement LoadFile2 initrd loading protocol for Linux

[Ard Biesheuvel]

On Mon, 26 Oct 2020 at 22:38, Atish Patra <atishp@atishpatra.org> wrote:
>
> On Sun, Oct 25, 2020 at 6:50 AM Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:
> >
> > Recent Linux kernels will invoke the LoadFile2 protocol installed on a
> > well-known vendor media devicepath to load the initrd if it is exposed
> > by the firmware. Using this method is preferred for two reasons:
> > - the Linux kernel is in charge of allocating the memory, and so it can
> >   implement any placement policy it wants (given that these tend to
> >   change between kernel versions),
> > - it is no longer necessary to modify the device tree provided by the
> >   firmware.
> >
> > So let's install this protocol when handling the 'initrd' command if
> > such a recent kernel was detected (based on the PE/COFF image version),
> > and defer loading the initrd contents until the point where the kernel
> > invokes the LoadFile2 protocol.
> >
>
> Thanks for adding the support for LoadFile2 protocol. This was one of
> the blockers for full RISC-V support.
> How do you prefer to proceed with RISC-V support ?
>

I'll defer to Daniel and Leif for the answer to that question.

> For RISC-V, we just need to move the arm64 loader to a common file so
> that both RISC-V/ARM64 can use it apart from
> few header file fixes. During the last attempt[1], I moved it to
> loader/efi/linux.c.
>

That seems the most logical to me. Given that it is shared between ARM
and arm64 today, it doesn't belong in grub-core/loader/arm64/linux.c
to begin with.

As I understand it, Daniel is doing a release imminently. Once that is
out of the way, I'm happy to proceed whichever way the maintainers
prefer.

> I am happy to test it on Qemu/hardware, if you want to send the series
> either part of this one or a separate series.
> I can rebase my previous series on top of this series as well. Please
> let me know your preference.
>

That would be a good start in any case, to ensure that the PE/COFF
image loading and LoadFile2 handling works as expected. And perhaps
simply merging the changes in that order is the most straight-forward.



> [1] https://www.mail-archive.com/grub-devel@gnu.org/msg30107.html
>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > ---
> >  grub-core/loader/arm64/linux.c | 109 +++++++++++++++++++-
> >  1 file changed, 108 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > index 28ff8584a3b5..c6a95e1f0c43 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -48,6 +48,10 @@ static grub_uint32_t cmdline_size;
> >  static grub_addr_t initrd_start;
> >  static grub_addr_t initrd_end;
> >
> > +static struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
> > +static grub_efi_handle_t initrd_lf2_handle;
> > +static int initrd_use_loadfile2;
> > +
> >  grub_err_t
> >  grub_arch_efi_linux_load_image_header (grub_file_t file,
> >                                        struct linux_arch_kernel_header * lh)
> > @@ -81,6 +85,18 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
> >         return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF image header");
> >      }
> >
> > +  /*
> > +   * Linux kernels built for any architecture are guaranteed to support the
> > +   * LoadFile2 based initrd loading protocol if the image version is >= 1.
> > +   */
> > +  if (lh->coff_image_header.optional_header.major_image_version >= 1)
> > +    initrd_use_loadfile2 = 1;
> > +  else
> > +    initrd_use_loadfile2 = 0;
> > +
> > +  grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n",
> > +               initrd_use_loadfile2 ? "en" : "dis");
> > +
> >    return GRUB_ERR_NONE;
> >  }
> >
> > @@ -250,13 +266,86 @@ allocate_initrd_mem (int initrd_pages)
> >                                        GRUB_EFI_LOADER_DATA);
> >  }
> >
> > +struct initrd_media_device_path {
> > +  grub_efi_vendor_media_device_path_t  vendor;
> > +  grub_efi_device_path_t               end;
> > +} GRUB_PACKED;
> > +
> > +#define LINUX_EFI_INITRD_MEDIA_GUID  \
> > +  { 0x5568e427, 0x68fc, 0x4f3d, \
> > +    { 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 } \
> > +  }
> > +
> > +static struct initrd_media_device_path initrd_lf2_device_path = {
> > +  {
> > +    {
> > +      GRUB_EFI_MEDIA_DEVICE_PATH_TYPE,
> > +      GRUB_EFI_VENDOR_MEDIA_DEVICE_PATH_SUBTYPE,
> > +      sizeof(grub_efi_vendor_media_device_path_t),
> > +    },
> > +    LINUX_EFI_INITRD_MEDIA_GUID
> > +  }, {
> > +    GRUB_EFI_END_DEVICE_PATH_TYPE,
> > +    GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > +    sizeof(grub_efi_device_path_t)
> > +  }
> > +};
> > +
> > +static grub_efi_status_t initrd_load_file2(grub_efi_load_file2_t *this,
> > +                                          grub_efi_device_path_t *device_path,
> > +                                          grub_efi_boolean_t boot_policy,
> > +                                          grub_efi_uintn_t *buffer_size,
> > +                                          void *buffer);
> > +
> > +static grub_efi_load_file2_t initrd_lf2 = {
> > +  initrd_load_file2
> > +};
> > +
> > +static grub_efi_status_t initrd_load_file2(grub_efi_load_file2_t *this,
> > +                                          grub_efi_device_path_t *device_path,
> > +                                          grub_efi_boolean_t boot_policy,
> > +                                          grub_efi_uintn_t *buffer_size,
> > +                                          void *buffer)
> > +{
> > +  grub_efi_status_t status = GRUB_EFI_SUCCESS;
> > +  grub_efi_uintn_t initrd_size;
> > +
> > +  if (!this || this != &initrd_lf2 || !buffer_size)
> > +    return GRUB_EFI_INVALID_PARAMETER;
> > +
> > +  if (device_path->type != GRUB_EFI_END_DEVICE_PATH_TYPE ||
> > +      device_path->subtype != GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE)
> > +    return GRUB_EFI_NOT_FOUND;
> > +
> > +  if (boot_policy)
> > +    return GRUB_EFI_UNSUPPORTED;
> > +
> > +  initrd_size = grub_get_initrd_size (&initrd_ctx);
> > +  if (!buffer || *buffer_size < initrd_size)
> > +    {
> > +      *buffer_size = initrd_size;
> > +      return GRUB_EFI_BUFFER_TOO_SMALL;
> > +    }
> > +
> > +  grub_dprintf ("linux", "Loading initrd via LOAD_FILE2_PROTOCOL\n");
> > +
> > +  if (grub_initrd_load (&initrd_ctx, NULL, buffer))
> > +    status = GRUB_EFI_LOAD_ERROR;
> > +
> > +  grub_initrd_close (&initrd_ctx);
> > +  return status;
> > +}
> > +
> >  static grub_err_t
> >  grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
> >                  int argc, char *argv[])
> >  {
> > -  struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
> >    int initrd_size, initrd_pages;
> >    void *initrd_mem = NULL;
> > +  grub_efi_guid_t load_file2_guid = GRUB_EFI_LOAD_FILE2_PROTOCOL_GUID;
> > +  grub_efi_guid_t device_path_guid = GRUB_EFI_DEVICE_PATH_GUID;
> > +  grub_efi_boot_services_t *b;
> > +  grub_efi_status_t status;
> >
> >    if (argc == 0)
> >      {
> > @@ -274,6 +363,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
> >    if (grub_initrd_init (argc, argv, &initrd_ctx))
> >      goto fail;
> >
> > +  if (initrd_use_loadfile2)
> > +    {
> > +      b = grub_efi_system_table->boot_services;
> > +      status = b->install_multiple_protocol_interfaces (&initrd_lf2_handle,
> > +                                                       &load_file2_guid,
> > +                                                       &initrd_lf2,
> > +                                                       &device_path_guid,
> > +                                                       &initrd_lf2_device_path,
> > +                                                       NULL);
> > +      if (status == GRUB_EFI_OUT_OF_RESOURCES)
> > +       {
> > +         grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> > +         return grub_errno;
> > +       }
> > +      grub_dprintf ("linux", "LoadFile2 initrd loading protocol installed\n");
> > +      return GRUB_ERR_NONE;
> > +    }
> > +
> >    initrd_size = grub_get_initrd_size (&initrd_ctx);
> >    grub_dprintf ("linux", "Loading initrd\n");
> >
> > --
> > 2.17.1
> >
>
>
> --
> Regards,
> Atish

Re: [PATCH v2 1/8] linux/arm: fix ARM Linux header layout

[Leif Lindholm]

On Sun, Oct 25, 2020 at 14:49:34 +0100, Ard Biesheuvel wrote:
> The hdr_offset member of the ARM Linux image header appears at
> offset 0x3c, matching the PE/COFF spec's placement of the COFF
> header offset in the MS-DOS header. We're currently off by four,
> so fix that.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  include/grub/arm/linux.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index 2e98a6689696..bcd5a7eb186e 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -30,7 +30,7 @@ struct linux_arm_kernel_header {
>    grub_uint32_t magic;
>    grub_uint32_t start; /* _start */
>    grub_uint32_t end;   /* _edata */
> -  grub_uint32_t reserved2[4];
> +  grub_uint32_t reserved2[3];
>    grub_uint32_t hdr_offset;

How did this ever work?

/
    Leif

>  };
>  
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/8] linux/arm: fix ARM Linux header layout

[Ard Biesheuvel]

On Wed, 4 Nov 2020 at 13:11, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Sun, Oct 25, 2020 at 14:49:34 +0100, Ard Biesheuvel wrote:
> > The hdr_offset member of the ARM Linux image header appears at
> > offset 0x3c, matching the PE/COFF spec's placement of the COFF
> > header offset in the MS-DOS header. We're currently off by four,
> > so fix that.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > ---
> >  include/grub/arm/linux.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> > index 2e98a6689696..bcd5a7eb186e 100644
> > --- a/include/grub/arm/linux.h
> > +++ b/include/grub/arm/linux.h
> > @@ -30,7 +30,7 @@ struct linux_arm_kernel_header {
> >    grub_uint32_t magic;
> >    grub_uint32_t start; /* _start */
> >    grub_uint32_t end;   /* _edata */
> > -  grub_uint32_t reserved2[4];
> > +  grub_uint32_t reserved2[3];
> >    grub_uint32_t hdr_offset;
>
> How did this ever work?
>

By ignoring the value of hdr_offset entirely everywhere else

Re: [PATCH v2 1/8] linux/arm: fix ARM Linux header layout

[Leif Lindholm]

On Wed, Nov 04, 2020 at 13:19:47 +0100, Ard Biesheuvel wrote:
> On Wed, 4 Nov 2020 at 13:11, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > On Sun, Oct 25, 2020 at 14:49:34 +0100, Ard Biesheuvel wrote:
> > > The hdr_offset member of the ARM Linux image header appears at
> > > offset 0x3c, matching the PE/COFF spec's placement of the COFF
> > > header offset in the MS-DOS header. We're currently off by four,
> > > so fix that.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > ---
> > >  include/grub/arm/linux.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> > > index 2e98a6689696..bcd5a7eb186e 100644
> > > --- a/include/grub/arm/linux.h
> > > +++ b/include/grub/arm/linux.h
> > > @@ -30,7 +30,7 @@ struct linux_arm_kernel_header {
> > >    grub_uint32_t magic;
> > >    grub_uint32_t start; /* _start */
> > >    grub_uint32_t end;   /* _edata */
> > > -  grub_uint32_t reserved2[4];
> > > +  grub_uint32_t reserved2[3];
> > >    grub_uint32_t hdr_offset;
> >
> > How did this ever work?
> >
> 
> By ignoring the value of hdr_offset entirely everywhere else

Oh, right - we only bother checking magic, doh!

/
    Leif

Re: [PATCH v2 1/8] linux/arm: fix ARM Linux header layout

[Daniel Kiper]

On Sun, Oct 25, 2020 at 02:49:34PM +0100, Ard Biesheuvel wrote:
> The hdr_offset member of the ARM Linux image header appears at
> offset 0x3c, matching the PE/COFF spec's placement of the COFF
> header offset in the MS-DOS header. We're currently off by four,
> so fix that.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

I was asked by Dimitri to take this patch. I double checked it. It
looks that it impacts only one grub_file_read() and one grub_dprintf().
So, there is no or minimal risk at this point and no big advantage
for upstream. Though, due to low risk, I will take it for downstream
advantage.

So, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Until I hear any objections in an hour or so...

Daniel

> ---
>  include/grub/arm/linux.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index 2e98a6689696..bcd5a7eb186e 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -30,7 +30,7 @@ struct linux_arm_kernel_header {
>    grub_uint32_t magic;
>    grub_uint32_t start; /* _start */
>    grub_uint32_t end;   /* _edata */
> -  grub_uint32_t reserved2[4];
> +  grub_uint32_t reserved2[3];
>    grub_uint32_t hdr_offset;
>  };
>
> --
> 2.17.1

Re: [PATCH v2 3/8] efi: move MS-DOS stub out of generic PE header definition

[Heinrich Schuchardt]

On 10/25/20 2:49 PM, Ard Biesheuvel wrote:
> The PE/COFF spec permits the COFF signature and file header to appear
> anywhere in the file, and the actual offset is recorded in 4 byte
> little endian field at offset 0x3c of the image.
>
> When GRUB is emitted as a PE/COFF binary, we reuse the 128 byte MS-DOS
> stub (even for non-x86 architectures), putting the COFF signature and
> file header at offset 0x80. However, other PE/COFF images may use
> different values, and non-x86 Linux kernels use an offset of 0x40
> instead.
>
> So let's get rid of the grub_pe32_header struct from pe32.h, given that
> it does not represent anything defined by the PE/COFF spec. Instead,
> use the GRUB_PE32_MSDOS_STUB_SIZE macro explicitly to reference the
> COFF header in the only place in the code where we rely on this.
>
> The remaining fields are moved into a struct grub_coff_image_header,
> which we will use later to access COFF header fields of arbitrary
> images (and which may therefore appear at different offsets)
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>   grub-core/kern/efi/efi.c | 5 +++--
>   include/grub/efi/pe32.h  | 5 +----
>   2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index e0165e74c587..9e5a72be538d 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -282,7 +282,7 @@ grub_addr_t
>   grub_efi_modules_addr (void)
>   {
>     grub_efi_loaded_image_t *image;
> -  struct grub_pe32_header *header;
> +  struct grub_coff_image_header *header;
>     struct grub_pe32_coff_header *coff_header;
>     struct grub_pe32_section_table *sections;
>     struct grub_pe32_section_table *section;
> @@ -293,7 +293,8 @@ grub_efi_modules_addr (void)
>     if (! image)
>       return 0;
>
> -  header = image->image_base;
> +  header = (struct grub_coff_image_header *) ((char *) image->image_base
> +					      + GRUB_PE32_MSDOS_STUB_SIZE);

After checking that the file starts with the letters 'MZ' we can find at
file offset 0x3c the position of the 'PE\0\0' indicating the start of
the COFF header. This avoids relying upon any fixed offset and conforms
to the PE Format specification.

See
https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#ms-dos-stub-image-only

Best regards

Heinrich

>     coff_header = &(header->coff_header);
>     sections
>       = (struct grub_pe32_section_table *) ((char *) coff_header
> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
> index 0ed8781f0376..a2da4b318c85 100644
> --- a/include/grub/efi/pe32.h
> +++ b/include/grub/efi/pe32.h
> @@ -254,11 +254,8 @@ struct grub_pe32_section_table
>
>   #define GRUB_PE32_SIGNATURE_SIZE 4
>
> -struct grub_pe32_header
> +struct grub_coff_image_header
>   {
> -  /* This should be filled in with GRUB_PE32_MSDOS_STUB.  */
> -  grub_uint8_t msdos_stub[GRUB_PE32_MSDOS_STUB_SIZE];
> -
>     /* This is always PE\0\0.  */
>     char signature[GRUB_PE32_SIGNATURE_SIZE];
>
>

Re: [PATCH v2 5/8] linux/arm: account for COFF headers appearing at unexpected offsets

[Heinrich Schuchardt]

On 10/25/20 2:49 PM, Ard Biesheuvel wrote:
> The way we load the Linux and PE/COFF image headers depends on a fixed
> placement of the COFF header at offset 0x40 into the file. This is a
> reasonable default, given that this is where Linux emits it today.
> However, in order to comply with the PE/COFF spec, which permits this
> header to appear anywhere in the file, let's ensure that we read the
> header from where it actually appears in the file if it is not located
> at offset 0x40.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>   grub-core/loader/arm64/linux.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 915b6ad7292d..28ff8584a3b5 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
>     grub_dprintf ("linux", "UEFI stub kernel:\n");
>     grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
>
> +  /*
> +   * The PE/COFF spec permits the COFF header to appear anywhere in the file, so
> +   * we need to double check whether it was where we expected it, and if not, we
> +   * must load it from the correct offset into the coff_image_header field of
> +   * struct linux_arch_kernel_header.
> +   */
> +  if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header)
> +    {
> +      grub_file_seek (file, lh->hdr_offset);

Isn't this overly complicated? Why don't we first read the whole file
into memory and then analyze it instead of using multiple accesses which
only slows down the process?

Best regards

Heinrich

> +
> +      if (grub_file_read (file, &lh->coff_image_header, sizeof(struct grub_coff_image_header))
> +	  != sizeof(struct grub_coff_image_header))
> +	return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF image header");
> +    }
> +
>     return GRUB_ERR_NONE;
>   }
>
>

Re: [PATCH v2 3/8] efi: move MS-DOS stub out of generic PE header definition

[Ard Biesheuvel]

On Thu, 8 Apr 2021 at 20:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/25/20 2:49 PM, Ard Biesheuvel wrote:
> > The PE/COFF spec permits the COFF signature and file header to appear
> > anywhere in the file, and the actual offset is recorded in 4 byte
> > little endian field at offset 0x3c of the image.
> >
> > When GRUB is emitted as a PE/COFF binary, we reuse the 128 byte MS-DOS
> > stub (even for non-x86 architectures), putting the COFF signature and
> > file header at offset 0x80. However, other PE/COFF images may use
> > different values, and non-x86 Linux kernels use an offset of 0x40
> > instead.
> >
> > So let's get rid of the grub_pe32_header struct from pe32.h, given that
> > it does not represent anything defined by the PE/COFF spec. Instead,
> > use the GRUB_PE32_MSDOS_STUB_SIZE macro explicitly to reference the
> > COFF header in the only place in the code where we rely on this.
> >
> > The remaining fields are moved into a struct grub_coff_image_header,
> > which we will use later to access COFF header fields of arbitrary
> > images (and which may therefore appear at different offsets)
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > ---
> >   grub-core/kern/efi/efi.c | 5 +++--
> >   include/grub/efi/pe32.h  | 5 +----
> >   2 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > index e0165e74c587..9e5a72be538d 100644
> > --- a/grub-core/kern/efi/efi.c
> > +++ b/grub-core/kern/efi/efi.c
> > @@ -282,7 +282,7 @@ grub_addr_t
> >   grub_efi_modules_addr (void)
> >   {
> >     grub_efi_loaded_image_t *image;
> > -  struct grub_pe32_header *header;
> > +  struct grub_coff_image_header *header;
> >     struct grub_pe32_coff_header *coff_header;
> >     struct grub_pe32_section_table *sections;
> >     struct grub_pe32_section_table *section;
> > @@ -293,7 +293,8 @@ grub_efi_modules_addr (void)
> >     if (! image)
> >       return 0;
> >
> > -  header = image->image_base;
> > +  header = (struct grub_coff_image_header *) ((char *) image->image_base
> > +                                           + GRUB_PE32_MSDOS_STUB_SIZE);
>
> After checking that the file starts with the letters 'MZ' we can find at
> file offset 0x3c the position of the 'PE\0\0' indicating the start of
> the COFF header. This avoids relying upon any fixed offset and conforms
> to the PE Format specification.
>

I think you might be missing the point here. This is not about
arbitrary PE/COFF images that GRUB loads, this is about the PE/COFF
image that is constructed by the GRUB build system to carry GRUB
itself: what would be the point of extracting the offset from the
image if we always put the PE header at the same offset?

In any case, I don't care deeply one way or the other, so feel free to
propose an alternative to this patch.


> See
> https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#ms-dos-stub-image-only
>
> Best regards
>
> Heinrich
>
> >     coff_header = &(header->coff_header);
> >     sections
> >       = (struct grub_pe32_section_table *) ((char *) coff_header
> > diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
> > index 0ed8781f0376..a2da4b318c85 100644
> > --- a/include/grub/efi/pe32.h
> > +++ b/include/grub/efi/pe32.h
> > @@ -254,11 +254,8 @@ struct grub_pe32_section_table
> >
> >   #define GRUB_PE32_SIGNATURE_SIZE 4
> >
> > -struct grub_pe32_header
> > +struct grub_coff_image_header
> >   {
> > -  /* This should be filled in with GRUB_PE32_MSDOS_STUB.  */
> > -  grub_uint8_t msdos_stub[GRUB_PE32_MSDOS_STUB_SIZE];
> > -
> >     /* This is always PE\0\0.  */
> >     char signature[GRUB_PE32_SIGNATURE_SIZE];
> >
> >
>

Re: [PATCH v2 5/8] linux/arm: account for COFF headers appearing at unexpected offsets

[Ard Biesheuvel]

On Thu, 8 Apr 2021 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/25/20 2:49 PM, Ard Biesheuvel wrote:
> > The way we load the Linux and PE/COFF image headers depends on a fixed
> > placement of the COFF header at offset 0x40 into the file. This is a
> > reasonable default, given that this is where Linux emits it today.
> > However, in order to comply with the PE/COFF spec, which permits this
> > header to appear anywhere in the file, let's ensure that we read the
> > header from where it actually appears in the file if it is not located
> > at offset 0x40.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > ---
> >   grub-core/loader/arm64/linux.c | 15 +++++++++++++++
> >   1 file changed, 15 insertions(+)
> >
> > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > index 915b6ad7292d..28ff8584a3b5 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
> >     grub_dprintf ("linux", "UEFI stub kernel:\n");
> >     grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
> >
> > +  /*
> > +   * The PE/COFF spec permits the COFF header to appear anywhere in the file, so
> > +   * we need to double check whether it was where we expected it, and if not, we
> > +   * must load it from the correct offset into the coff_image_header field of
> > +   * struct linux_arch_kernel_header.
> > +   */
> > +  if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header)
> > +    {
> > +      grub_file_seek (file, lh->hdr_offset);
>
> Isn't this overly complicated? Why don't we first read the whole file
> into memory and then analyze it instead of using multiple accesses which
> only slows down the process?
>

Given that the condition will never hold in practice, as the offset is
always going to be 0x40, this change is not expected to affect
performance at all.

Doing a complete overhaul of the PE image loading logic for this seems
unwise to me.

Re: [PATCH v2 3/8] efi: move MS-DOS stub out of generic PE header definition

[Heinrich Schuchardt]

On 4/9/21 8:10 AM, Ard Biesheuvel wrote:
> On Thu, 8 Apr 2021 at 20:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/25/20 2:49 PM, Ard Biesheuvel wrote:
>>> The PE/COFF spec permits the COFF signature and file header to appear
>>> anywhere in the file, and the actual offset is recorded in 4 byte
>>> little endian field at offset 0x3c of the image.
>>>
>>> When GRUB is emitted as a PE/COFF binary, we reuse the 128 byte MS-DOS
>>> stub (even for non-x86 architectures), putting the COFF signature and
>>> file header at offset 0x80. However, other PE/COFF images may use
>>> different values, and non-x86 Linux kernels use an offset of 0x40
>>> instead.
>>>
>>> So let's get rid of the grub_pe32_header struct from pe32.h, given that
>>> it does not represent anything defined by the PE/COFF spec. Instead,
>>> use the GRUB_PE32_MSDOS_STUB_SIZE macro explicitly to reference the
>>> COFF header in the only place in the code where we rely on this.
>>>
>>> The remaining fields are moved into a struct grub_coff_image_header,
>>> which we will use later to access COFF header fields of arbitrary
>>> images (and which may therefore appear at different offsets)
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> ---
>>>    grub-core/kern/efi/efi.c | 5 +++--
>>>    include/grub/efi/pe32.h  | 5 +----
>>>    2 files changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
>>> index e0165e74c587..9e5a72be538d 100644
>>> --- a/grub-core/kern/efi/efi.c
>>> +++ b/grub-core/kern/efi/efi.c
>>> @@ -282,7 +282,7 @@ grub_addr_t
>>>    grub_efi_modules_addr (void)
>>>    {
>>>      grub_efi_loaded_image_t *image;
>>> -  struct grub_pe32_header *header;
>>> +  struct grub_coff_image_header *header;
>>>      struct grub_pe32_coff_header *coff_header;
>>>      struct grub_pe32_section_table *sections;
>>>      struct grub_pe32_section_table *section;
>>> @@ -293,7 +293,8 @@ grub_efi_modules_addr (void)
>>>      if (! image)
>>>        return 0;
>>>
>>> -  header = image->image_base;
>>> +  header = (struct grub_coff_image_header *) ((char *) image->image_base
>>> +                                           + GRUB_PE32_MSDOS_STUB_SIZE);
>>
>> After checking that the file starts with the letters 'MZ' we can find at
>> file offset 0x3c the position of the 'PE\0\0' indicating the start of
>> the COFF header. This avoids relying upon any fixed offset and conforms
>> to the PE Format specification.
>>
>
> I think you might be missing the point here. This is not about
> arbitrary PE/COFF images that GRUB loads, this is about the PE/COFF
> image that is constructed by the GRUB build system to carry GRUB
> itself: what would be the point of extracting the offset from the
> image if we always put the PE header at the same offset?

We should not assume that a file /boot/grub/x86_64-efi/core.efi contains
a GRUB EFI binary. We should check the header. If we can't find 'MZ' at
offset 0 and 'PE\0\0' at the position indicated by the offset at 0x3c,
it is definitively not a valid EFI binary.

I would prefer one common function for checking EFI headers instead of
grub_arch_efi_linux_check_image() per architecture.

Best regards

Heinrich

>
> In any case, I don't care deeply one way or the other, so feel free to
> propose an alternative to this patch.
>
>
>> See
>> https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#ms-dos-stub-image-only
>>
>> Best regards
>>
>> Heinrich
>>
>>>      coff_header = &(header->coff_header);
>>>      sections
>>>        = (struct grub_pe32_section_table *) ((char *) coff_header
>>> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
>>> index 0ed8781f0376..a2da4b318c85 100644
>>> --- a/include/grub/efi/pe32.h
>>> +++ b/include/grub/efi/pe32.h
>>> @@ -254,11 +254,8 @@ struct grub_pe32_section_table
>>>
>>>    #define GRUB_PE32_SIGNATURE_SIZE 4
>>>
>>> -struct grub_pe32_header
>>> +struct grub_coff_image_header
>>>    {
>>> -  /* This should be filled in with GRUB_PE32_MSDOS_STUB.  */
>>> -  grub_uint8_t msdos_stub[GRUB_PE32_MSDOS_STUB_SIZE];
>>> -
>>>      /* This is always PE\0\0.  */
>>>      char signature[GRUB_PE32_SIGNATURE_SIZE];
>>>
>>>
>>

Re: [PATCH v2 5/8] linux/arm: account for COFF headers appearing at unexpected offsets

[Heinrich Schuchardt]

On 4/9/21 8:12 AM, Ard Biesheuvel wrote:
> On Thu, 8 Apr 2021 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/25/20 2:49 PM, Ard Biesheuvel wrote:
>>> The way we load the Linux and PE/COFF image headers depends on a fixed
>>> placement of the COFF header at offset 0x40 into the file. This is a
>>> reasonable default, given that this is where Linux emits it today.
>>> However, in order to comply with the PE/COFF spec, which permits this
>>> header to appear anywhere in the file, let's ensure that we read the
>>> header from where it actually appears in the file if it is not located
>>> at offset 0x40.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> ---
>>>    grub-core/loader/arm64/linux.c | 15 +++++++++++++++
>>>    1 file changed, 15 insertions(+)
>>>
>>> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
>>> index 915b6ad7292d..28ff8584a3b5 100644
>>> --- a/grub-core/loader/arm64/linux.c
>>> +++ b/grub-core/loader/arm64/linux.c
>>> @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
>>>      grub_dprintf ("linux", "UEFI stub kernel:\n");
>>>      grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
>>>
>>> +  /*
>>> +   * The PE/COFF spec permits the COFF header to appear anywhere in the file, so
>>> +   * we need to double check whether it was where we expected it, and if not, we
>>> +   * must load it from the correct offset into the coff_image_header field of
>>> +   * struct linux_arch_kernel_header.
>>> +   */
>>> +  if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header)
>>> +    {
>>> +      grub_file_seek (file, lh->hdr_offset);
>>
>> Isn't this overly complicated? Why don't we first read the whole file
>> into memory and then analyze it instead of using multiple accesses which
>> only slows down the process?
>>
>
> Given that the condition will never hold in practice, as the offset is
> always going to be 0x40, this change is not expected to affect
> performance at all.

The PE COFF specification let's you specify any value. The linux command
can be used to launch arbitrary EFI binaries if they have the Linux
magic 'ARM\x64' in the right place.

What I never understood is why the linux command is checking this Linux
magic field at all instead of running any EFI binary thrown at it.

Best regards

Heinrich

>
> Doing a complete overhaul of the PE image loading logic for this seems
> unwise to me.
>

Re: [PATCH v2 5/8] linux/arm: account for COFF headers appearing at unexpected offsets

[Ard Biesheuvel]

On Fri, 9 Apr 2021 at 08:40, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/9/21 8:12 AM, Ard Biesheuvel wrote:
> > On Thu, 8 Apr 2021 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/25/20 2:49 PM, Ard Biesheuvel wrote:
> >>> The way we load the Linux and PE/COFF image headers depends on a fixed
> >>> placement of the COFF header at offset 0x40 into the file. This is a
> >>> reasonable default, given that this is where Linux emits it today.
> >>> However, in order to comply with the PE/COFF spec, which permits this
> >>> header to appear anywhere in the file, let's ensure that we read the
> >>> header from where it actually appears in the file if it is not located
> >>> at offset 0x40.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> >>> ---
> >>>    grub-core/loader/arm64/linux.c | 15 +++++++++++++++
> >>>    1 file changed, 15 insertions(+)
> >>>
> >>> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> >>> index 915b6ad7292d..28ff8584a3b5 100644
> >>> --- a/grub-core/loader/arm64/linux.c
> >>> +++ b/grub-core/loader/arm64/linux.c
> >>> @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
> >>>      grub_dprintf ("linux", "UEFI stub kernel:\n");
> >>>      grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
> >>>
> >>> +  /*
> >>> +   * The PE/COFF spec permits the COFF header to appear anywhere in the file, so
> >>> +   * we need to double check whether it was where we expected it, and if not, we
> >>> +   * must load it from the correct offset into the coff_image_header field of
> >>> +   * struct linux_arch_kernel_header.
> >>> +   */
> >>> +  if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header)
> >>> +    {
> >>> +      grub_file_seek (file, lh->hdr_offset);
> >>
> >> Isn't this overly complicated? Why don't we first read the whole file
> >> into memory and then analyze it instead of using multiple accesses which
> >> only slows down the process?
> >>
> >
> > Given that the condition will never hold in practice, as the offset is
> > always going to be 0x40, this change is not expected to affect
> > performance at all.
>
> The PE COFF specification let's you specify any value. The linux command
> can be used to launch arbitrary EFI binaries if they have the Linux
> magic 'ARM\x64' in the right place.
>
> What I never understood is why the linux command is checking this Linux
> magic field at all instead of running any EFI binary thrown at it.
>

I don't disagree with you on that. The question is whether it should
be in scope for this change to fix all of that.