[PATCH v4 0/6] linux: implement LoadFile2 initrd loading

[PATCH v4 0/6] linux: implement LoadFile2 initrd loading

[Ard Biesheuvel]

This implements the LoadFile2 initrd loading protocol, which is
essentially a callback interface 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. This is especially
relevant for the upcoming LoongArch support, which does not use either
DT or struct bootparams, and would have to rely on the initrd= command
line interface, which is deprecated and of limited utility [0].

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]

Changes since v3:
- drop a couple of patches that have been merged independently in the
  meantime
- update patch #1 to read the PE image header offset from the file
  instead of using the harcoded offset
- add acks from Heinrich and Ilias

Changes since v2:
- incorporate some ancient feedback from Daniel that I never saw until
  today. (this is why I am sending two versions of the same series on
  the same day - apologies for the spam)

[0] The initrd= command line loader can only access files that reside on
the same volume as the loaded image, which means GRUB would have to
present this volume abstraction in order to serve the initrd file.
Another reason why this method is problematic is generic EFI zboot,
which is being added to Linux, and which calls loadimage on another,
embedded PE/COFF image which would also need to expose this volume
abstraction.

Cc: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Nikita Ermakov <arei@altlinux.org>
Cc: Atish Patra <atishp@atishpatra.org>
Cc: Huacai Chen <chenhuacai@loongson.cn>
Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Cc: dann frazier <dann.frazier@canonical.com>
Cc: Julian Andres Klode <julian.klode@canonical.com>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Ard Biesheuvel (6):
  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/efinet: Don't close connections at fini_hw() time
  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           |   8 +-
 grub-core/loader/arm64/linux.c     | 172 ++++++++++++++++++--
 grub-core/loader/arm64/xen_boot.c  |  23 +--
 grub-core/loader/efi/fdt.c         |   7 +-
 grub-core/net/drivers/efi/efinet.c |  10 +-
 grub-core/net/net.c                |   2 +-
 include/grub/arm/linux.h           |   6 +
 include/grub/arm64/linux.h         |   4 +
 include/grub/efi/api.h             |  40 +++++
 include/grub/efi/efi.h             |   4 +-
 include/grub/efi/pe32.h            |  16 +-
 include/grub/net.h                 |   3 +-
 13 files changed, 248 insertions(+), 48 deletions(-)

-- 
2.35.1

[PATCH v4 1/6] 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,
introduce a minimal struct grub_msdos_image_header type based on the
PE/COFF spec's description of the image header, and use the offset
recorded at file position 0x3c to discover the actual location of the PE
signature and the COFF image header.

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 <ardb@kernel.org>
---
 grub-core/kern/efi/efi.c |  8 ++++++--
 include/grub/efi/pe32.h  | 16 ++++++++++++----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index e8a976a22f15..f85587d66635 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -302,7 +302,8 @@ grub_addr_t
 grub_efi_modules_addr (void)
 {
   grub_efi_loaded_image_t *image;
-  struct grub_pe32_header *header;
+  struct grub_msdos_image_header *dos_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;
@@ -313,7 +314,10 @@ grub_efi_modules_addr (void)
   if (! image)
     return 0;
 
-  header = image->image_base;
+  dos_header = (struct grub_msdos_image_header *)image->image_base;
+
+  header = (struct grub_coff_image_header *) ((char *) dos_header
+                                              + dos_header->pe_signature_offset);
   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..6688d96c0046 100644
--- a/include/grub/efi/pe32.h
+++ b/include/grub/efi/pe32.h
@@ -48,6 +48,17 @@
 
 #define GRUB_PE32_MAGIC			0x5a4d
 
+struct grub_msdos_image_header
+{
+  /* This is always 'MZ'. (GRUB_PE32_MAGIC)  */
+  grub_uint16_t msdos_magic;
+
+  grub_uint16_t reserved[29];
+
+  /* The file offset of the PE signature and COFF image header. */
+  grub_uint32_t pe_signature_offset;
+};
+
 /* According to the spec, the minimal alignment is 512 bytes...
    But some examples (such as EFI drivers in the Intel
    Sample Implementation) use 32 bytes (0x20) instead, and it seems
@@ -254,11 +265,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.35.1

[PATCH v4 2/6] 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 <ardb@kernel.org>
---
 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 b5b559c236e0..7c0f17cf933d 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->code0 & 0xffff) != GRUB_PE32_MAGIC)
     return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
 		       N_("plain image kernel not supported - rebuild with CONFIG_(U)EFI_STUB enabled"));
@@ -301,10 +306,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 7e22b4ab6990..c5ea9e8f503a 100644
--- a/include/grub/arm64/linux.h
+++ b/include/grub/arm64/linux.h
@@ -21,6 +21,8 @@
 
 #include <grub/types.h>
 
+#include <grub/efi/pe32.h>
+
 #define GRUB_LINUX_ARM64_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
 
 /* From linux/Documentation/arm64/booting.txt */
@@ -36,6 +38,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 eb2dfdfce9f8..e61272de5330 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -102,7 +102,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.35.1

[PATCH v4 3/6] 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 allows 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 <ardb@kernel.org>
---
 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 7c0f17cf933d..56ba8d0a6ea3 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -63,6 +63,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.35.1

[PATCH v4 4/6] efi/efinet: Don't close connections at fini_hw() time

[Ard Biesheuvel]

When GRUB runs on top of EFI firmware, it only has access to block and
network device abstractions exposed by the firmware, and it is up to the
firmware to quiesce the underlying hardware when exiting boot services
and handing over to the OS.

This is especially important for network devices, to prevent incoming
packets from being DMA'd straight into memory after the OS has taken
over but before it has managed to reconfigure the network hardware.

GRUB handles this by means of the grub_net_fini_hw() preboot hook, which
is executed before calling into the booted image. This means that all
network devices disappear or become inoperable before the EFI stub
executes on EFI targeted builds. This is problematic as it prevents the
EFI stub from calling back into GRUB provided protocols such as
LoadFile2 for the initrd, which we will provide in a subsequent patch.

So add a flag that indicates to the network core that EFI network
devices should not be closed when grub_net_fini_hw() is called.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 grub-core/net/drivers/efi/efinet.c | 10 +++++++++-
 grub-core/net/net.c                |  2 +-
 include/grub/net.h                 |  3 ++-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
index 73343d26d9e1..5adf5f40f492 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -320,7 +320,15 @@ grub_efinet_findcards (void)
 
       card->name = grub_xasprintf ("efinet%d", i++);
       card->driver = &efidriver;
-      card->flags = 0;
+      /*
+       * EFI network devices are abstract SNP protocol instances, and the
+       * firmware is in charge of ensuring that they will be torn down when the
+       * OS loader hands off to the OS proper. Closing them as part of the
+       * preboot cleanup is therefore unnecessary, and undesirable, as it
+       * prevents us from using the network connection in a protocal callback
+       * such as LoadFile2 for initrd loading.
+       */
+      card->flags = GRUB_NET_CARD_NO_CLOSE_ON_FINI_HW;
       card->default_address.type = GRUB_NET_LINK_LEVEL_PROTOCOL_ETHERNET;
       grub_memcpy (card->default_address.mac,
 		   net->mode->current_address,
diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 064e7114e012..7046dc57890a 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -1787,7 +1787,7 @@ grub_net_fini_hw (int noreturn __attribute__ ((unused)))
 {
   struct grub_net_card *card;
   FOR_NET_CARDS (card)
-    if (card->opened)
+    if (card->opened && !(card->flags & GRUB_NET_CARD_NO_CLOSE_ON_FINI_HW))
       {
 	if (card->driver->close)
 	  card->driver->close (card);
diff --git a/include/grub/net.h b/include/grub/net.h
index a64a04cc80b1..79cba357ae6a 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -64,7 +64,8 @@ typedef enum grub_net_interface_flags
 typedef enum grub_net_card_flags
   {
     GRUB_NET_CARD_HWADDRESS_IMMUTABLE = 1,
-    GRUB_NET_CARD_NO_MANUAL_INTERFACES = 2
+    GRUB_NET_CARD_NO_MANUAL_INTERFACES = 2,
+    GRUB_NET_CARD_NO_CLOSE_ON_FINI_HW = 4
   } grub_net_card_flags_t;
 
 struct grub_net_card;
-- 
2.35.1

[PATCH v4 5/6] 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 path 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 <ardb@kernel.org>
Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 grub-core/commands/efi/lsefi.c |   1 +
 grub-core/loader/arm64/linux.c | 123 +++++++++++++++++++-
 include/grub/efi/api.h         |  40 +++++++
 3 files changed, 163 insertions(+), 1 deletion(-)

diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c
index f46ba3b49384..c304d25ccdd6 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 file2" },
     { 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/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index 56ba8d0a6ea3..8b7fbb35fb72 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -48,6 +48,39 @@ 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 = NULL;
+static int initrd_use_loadfile2 = 0;
+
+static grub_efi_guid_t load_file2_guid = GRUB_EFI_LOAD_FILE2_PROTOCOL_GUID;
+static grub_efi_guid_t device_path_guid = GRUB_EFI_DEVICE_PATH_GUID;
+
+static initrd_media_device_path_t 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 __grub_efi_api
+grub_efi_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 = {
+  grub_efi_initrd_load_file2
+};
+
 grub_err_t
 grub_arch_efi_linux_load_image_header (grub_file_t file,
                                       struct linux_arch_kernel_header * lh)
@@ -78,6 +111,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;
 }
 
@@ -197,6 +242,8 @@ grub_linux_boot (void)
 static grub_err_t
 grub_linux_unload (void)
 {
+  grub_efi_boot_services_t *b = grub_efi_system_table->boot_services;
+
   grub_dl_unref (my_mod);
   loaded = 0;
   if (initrd_start)
@@ -208,6 +255,18 @@ grub_linux_unload (void)
     grub_efi_free_pages ((grub_addr_t) kernel_addr,
 			 GRUB_EFI_BYTES_TO_PAGES (kernel_size));
   grub_fdt_unload ();
+
+  if (initrd_lf2_handle)
+    {
+      b->uninstall_multiple_protocol_interfaces (initrd_lf2_handle,
+                                                 &load_file2_guid,
+                                                 &initrd_lf2,
+                                                 &device_path_guid,
+                                                 &initrd_lf2_device_path,
+                                                 NULL);
+      initrd_lf2_handle = NULL;
+      initrd_use_loadfile2 = 0;
+    }
   return GRUB_ERR_NONE;
 }
 
@@ -247,13 +306,50 @@ allocate_initrd_mem (int initrd_pages)
 				       GRUB_EFI_LOADER_DATA);
 }
 
+static grub_efi_status_t __grub_efi_api
+grub_efi_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 != &initrd_lf2 || buffer_size == NULL)
+    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 == NULL || *buffer_size < initrd_size)
+    {
+      *buffer_size = initrd_size;
+      return GRUB_EFI_BUFFER_TOO_SMALL;
+    }
+
+  grub_dprintf ("linux", "Providing initrd via EFI_LOAD_FILE2_PROTOCOL\n");
+
+  if (grub_initrd_load (&initrd_ctx, buffer))
+    status = GRUB_EFI_DEVICE_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_boot_services_t *b = grub_efi_system_table->boot_services;
+  grub_efi_status_t status;
 
   if (argc == 0)
     {
@@ -271,6 +367,31 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
   if (grub_initrd_init (argc, argv, &initrd_ctx))
     goto fail;
 
+  if (initrd_use_loadfile2)
+    {
+      if (!initrd_lf2_handle)
+        {
+          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"));
+              goto fail;
+            }
+          else if (status != GRUB_EFI_SUCCESS)
+            {
+              grub_error (GRUB_ERR_BAD_ARGUMENT, N_("failed to install protocols"));
+              goto fail;
+            }
+        }
+      grub_dprintf ("linux", "Using LoadFile2 initrd loading protocol\n");
+      return GRUB_ERR_NONE;
+    }
+
   initrd_size = grub_get_initrd_size (&initrd_ctx);
   grub_dprintf ("linux", "Loading initrd\n");
 
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 1ef4046225cb..58b13b8c66b5 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 } \
@@ -359,6 +364,11 @@
     { 0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44 } \
   }
 
+#define LINUX_EFI_INITRD_MEDIA_GUID  \
+  { 0x5568e427, 0x68fc, 0x4f3d, \
+    { 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 } \
+  }
+
 struct grub_efi_sal_system_table
 {
   grub_uint32_t signature;
@@ -559,6 +569,20 @@ typedef grub_uint16_t grub_efi_char16_t;
 
 typedef grub_efi_uintn_t grub_efi_status_t;
 
+/*
+ * On x86, the EFI calling convention may deviate from the local one, so
+ * callback functions exposed to the firmware must carry the follow attribute
+ * annotation. (This includes protocols implemented by GRUB that are installed
+ * into the EFI protocol database.)
+ */
+#if defined(__i386__)
+#define __grub_efi_api			__attribute__((regparm(0)))
+#elif defined(__x86_64__)
+#define __grub_efi_api			__attribute__((ms_abi))
+#else
+#define __grub_efi_api
+#endif
+
 #define GRUB_EFI_ERROR_CODE(value)	\
   ((((grub_efi_status_t) 1) << (sizeof (grub_efi_status_t) * 8 - 1)) | (value))
 
@@ -1749,6 +1773,22 @@ struct grub_efi_rng_protocol
 };
 typedef struct grub_efi_rng_protocol grub_efi_rng_protocol_t;
 
+struct grub_efi_load_file2
+{
+  grub_efi_status_t (__grub_efi_api *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;
+
+struct initrd_media_device_path {
+  grub_efi_vendor_media_device_path_t  vendor;
+  grub_efi_device_path_t               end;
+} GRUB_PACKED;
+typedef struct initrd_media_device_path initrd_media_device_path_t;
+
 #if (GRUB_TARGET_SIZEOF_VOID_P == 4) || defined (__ia64__) \
   || defined (__aarch64__) || defined (__MINGW64__) || defined (__CYGWIN__) \
   || defined(__riscv)
-- 
2.35.1

[PATCH v4 6/6] 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 that when the LoadFile2 protocol is being
used, there is no 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 <ardb@kernel.org>
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.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 8b7fbb35fb72..36396e9f7ff5 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -133,21 +133,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 80d70887474a..fd8093ac83c9 100644
--- a/grub-core/loader/efi/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -89,13 +89,16 @@ grub_fdt_install (void)
   static 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.35.1

Re: [PATCH v4 1/6] efi: move MS-DOS stub out of generic PE header definition

[Leif Lindholm]

On Thu, Sep 08, 2022 at 15:30:12 +0200, 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,
> introduce a minimal struct grub_msdos_image_header type based on the
> PE/COFF spec's description of the image header, and use the offset
> recorded at file position 0x3c to discover the actual location of the PE
> signature and the COFF image header.
> 
> 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 <ardb@kernel.org>
> ---
>  grub-core/kern/efi/efi.c |  8 ++++++--
>  include/grub/efi/pe32.h  | 16 ++++++++++++----
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index e8a976a22f15..f85587d66635 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -302,7 +302,8 @@ grub_addr_t
>  grub_efi_modules_addr (void)
>  {
>    grub_efi_loaded_image_t *image;
> -  struct grub_pe32_header *header;
> +  struct grub_msdos_image_header *dos_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;
> @@ -313,7 +314,10 @@ grub_efi_modules_addr (void)
>    if (! image)
>      return 0;
>  
> -  header = image->image_base;
> +  dos_header = (struct grub_msdos_image_header *)image->image_base;
> +
> +  header = (struct grub_coff_image_header *) ((char *) dos_header
> +                                              + dos_header->pe_signature_offset);
>    coff_header = &(header->coff_header);

This is where I get semantically confused.
We now have a coff_image_header called header (pointing at the space
for the PE\0\0 signature, which comes immediately before the COFF
header, and isn't part of it).

And then we have a pe32_coff_header called coff_header, pointing at
the machine field (the start) of the COFF header.

Since "header" is no longer referring to an image header, could we
chuck out the signature as well from the structure and add
GRUB_PE32_SIGNATURE_SIZE when calculating?

I.e. drop "header" altogether and do:

  dos_header = (struct grub_msdos_image_header *)image->image_base;

  coff_header = (struct grub_coff_image_header *) ((char *) dos_header
                                              + dos_header->pe_signature_offset
					      + GRUB_PE32_SIGNATURE_SIZE
					      );
?

/
    Leif

>    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..6688d96c0046 100644
> --- a/include/grub/efi/pe32.h
> +++ b/include/grub/efi/pe32.h
> @@ -48,6 +48,17 @@
>  
>  #define GRUB_PE32_MAGIC			0x5a4d
>  
> +struct grub_msdos_image_header
> +{
> +  /* This is always 'MZ'. (GRUB_PE32_MAGIC)  */
> +  grub_uint16_t msdos_magic;
> +
> +  grub_uint16_t reserved[29];
> +
> +  /* The file offset of the PE signature and COFF image header. */
> +  grub_uint32_t pe_signature_offset;
> +};
> +
>  /* According to the spec, the minimal alignment is 512 bytes...
>     But some examples (such as EFI drivers in the Intel
>     Sample Implementation) use 32 bytes (0x20) instead, and it seems
> @@ -254,11 +265,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.35.1
> 

Re: [PATCH v4 1/6] efi: move MS-DOS stub out of generic PE header definition

[Ard Biesheuvel]

On Thu, 15 Sept 2022 at 14:21, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Thu, Sep 08, 2022 at 15:30:12 +0200, 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,
> > introduce a minimal struct grub_msdos_image_header type based on the
> > PE/COFF spec's description of the image header, and use the offset
> > recorded at file position 0x3c to discover the actual location of the PE
> > signature and the COFF image header.
> >
> > 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 <ardb@kernel.org>
> > ---
> >  grub-core/kern/efi/efi.c |  8 ++++++--
> >  include/grub/efi/pe32.h  | 16 ++++++++++++----
> >  2 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > index e8a976a22f15..f85587d66635 100644
> > --- a/grub-core/kern/efi/efi.c
> > +++ b/grub-core/kern/efi/efi.c
> > @@ -302,7 +302,8 @@ grub_addr_t
> >  grub_efi_modules_addr (void)
> >  {
> >    grub_efi_loaded_image_t *image;
> > -  struct grub_pe32_header *header;
> > +  struct grub_msdos_image_header *dos_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;
> > @@ -313,7 +314,10 @@ grub_efi_modules_addr (void)
> >    if (! image)
> >      return 0;
> >
> > -  header = image->image_base;
> > +  dos_header = (struct grub_msdos_image_header *)image->image_base;
> > +
> > +  header = (struct grub_coff_image_header *) ((char *) dos_header
> > +                                              + dos_header->pe_signature_offset);
> >    coff_header = &(header->coff_header);
>
> This is where I get semantically confused.
> We now have a coff_image_header called header (pointing at the space
> for the PE\0\0 signature, which comes immediately before the COFF
> header, and isn't part of it).
>
> And then we have a pe32_coff_header called coff_header, pointing at
> the machine field (the start) of the COFF header.
>
> Since "header" is no longer referring to an image header, could we
> chuck out the signature as well from the structure and add
> GRUB_PE32_SIGNATURE_SIZE when calculating?
>
> I.e. drop "header" altogether and do:
>
>   dos_header = (struct grub_msdos_image_header *)image->image_base;
>
>   coff_header = (struct grub_coff_image_header *) ((char *) dos_header
>                                               + dos_header->pe_signature_offset
>                                               + GRUB_PE32_SIGNATURE_SIZE
>                                               );
> ?
>

I suppose that should work, but it does mean we have to add  a 'char
signature[GRUB_PE32_SIGNATURE_SIZE];' member to the existing structs
that incorporate grub_coff_image_header, along with adding
GRUB_PE32_SIGNATURE_SIZE every time we refer to the PE header offset.


> >    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..6688d96c0046 100644
> > --- a/include/grub/efi/pe32.h
> > +++ b/include/grub/efi/pe32.h
> > @@ -48,6 +48,17 @@
> >
> >  #define GRUB_PE32_MAGIC                      0x5a4d
> >
> > +struct grub_msdos_image_header
> > +{
> > +  /* This is always 'MZ'. (GRUB_PE32_MAGIC)  */
> > +  grub_uint16_t msdos_magic;
> > +
> > +  grub_uint16_t reserved[29];
> > +
> > +  /* The file offset of the PE signature and COFF image header. */
> > +  grub_uint32_t pe_signature_offset;
> > +};
> > +
> >  /* According to the spec, the minimal alignment is 512 bytes...
> >     But some examples (such as EFI drivers in the Intel
> >     Sample Implementation) use 32 bytes (0x20) instead, and it seems
> > @@ -254,11 +265,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.35.1
> >

Re: [PATCH v4 1/6] efi: move MS-DOS stub out of generic PE header definition

[Daniel Kiper]

On Thu, Sep 15, 2022 at 04:03:36PM +0200, Ard Biesheuvel wrote:
> On Thu, 15 Sept 2022 at 14:21, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > On Thu, Sep 08, 2022 at 15:30:12 +0200, 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,
> > > introduce a minimal struct grub_msdos_image_header type based on the
> > > PE/COFF spec's description of the image header, and use the offset
> > > recorded at file position 0x3c to discover the actual location of the PE
> > > signature and the COFF image header.
> > >
> > > 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 <ardb@kernel.org>
> > > ---
> > >  grub-core/kern/efi/efi.c |  8 ++++++--
> > >  include/grub/efi/pe32.h  | 16 ++++++++++++----
> > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > > index e8a976a22f15..f85587d66635 100644
> > > --- a/grub-core/kern/efi/efi.c
> > > +++ b/grub-core/kern/efi/efi.c
> > > @@ -302,7 +302,8 @@ grub_addr_t
> > >  grub_efi_modules_addr (void)
> > >  {
> > >    grub_efi_loaded_image_t *image;
> > > -  struct grub_pe32_header *header;
> > > +  struct grub_msdos_image_header *dos_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;
> > > @@ -313,7 +314,10 @@ grub_efi_modules_addr (void)
> > >    if (! image)
> > >      return 0;
> > >
> > > -  header = image->image_base;
> > > +  dos_header = (struct grub_msdos_image_header *)image->image_base;
> > > +
> > > +  header = (struct grub_coff_image_header *) ((char *) dos_header
> > > +                                              + dos_header->pe_signature_offset);
> > >    coff_header = &(header->coff_header);
> >
> > This is where I get semantically confused.
> > We now have a coff_image_header called header (pointing at the space
> > for the PE\0\0 signature, which comes immediately before the COFF
> > header, and isn't part of it).
> >
> > And then we have a pe32_coff_header called coff_header, pointing at
> > the machine field (the start) of the COFF header.
> >
> > Since "header" is no longer referring to an image header, could we
> > chuck out the signature as well from the structure and add
> > GRUB_PE32_SIGNATURE_SIZE when calculating?
> >
> > I.e. drop "header" altogether and do:
> >
> >   dos_header = (struct grub_msdos_image_header *)image->image_base;
> >
> >   coff_header = (struct grub_coff_image_header *) ((char *) dos_header
> >                                               + dos_header->pe_signature_offset
> >                                               + GRUB_PE32_SIGNATURE_SIZE
> >                                               );
> > ?
> >
>
> I suppose that should work, but it does mean we have to add  a 'char
> signature[GRUB_PE32_SIGNATURE_SIZE];' member to the existing structs
> that incorporate grub_coff_image_header, along with adding
> GRUB_PE32_SIGNATURE_SIZE every time we refer to the PE header offset.

After checking the PE/COFF spec it seems to me the grub_coff_image_header
should be renamed to grub_pe_image_header. Then everything should be OK.

Daniel

Re: [PATCH v4 2/6] linux/arm: unify ARM/arm64 vs Xen PE/COFF header handling

[Daniel Kiper]

On Thu, Sep 08, 2022 at 03:30:13PM +0200, Ard Biesheuvel wrote:
> 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 <ardb@kernel.org>
> ---
>  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 b5b559c236e0..7c0f17cf933d 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))

s/(long)/(grub_ssize_t)/

Please mention this change in the commit message.

> +    return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read Linux image header");
> +
>    if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
>      return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
>  		       N_("plain image kernel not supported - rebuild with CONFIG_(U)EFI_STUB enabled"));
> @@ -301,10 +306,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;
> +

Please drop this empty line.

> +#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 7e22b4ab6990..c5ea9e8f503a 100644
> --- a/include/grub/arm64/linux.h
> +++ b/include/grub/arm64/linux.h
> @@ -21,6 +21,8 @@
>
>  #include <grub/types.h>
>

Please drop this empty line.

> +#include <grub/efi/pe32.h>
> +
>  #define GRUB_LINUX_ARM64_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
>
>  /* From linux/Documentation/arm64/booting.txt */
> @@ -36,6 +38,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 */
> +

Please drop this empty line.

> +  struct grub_coff_image_header coff_image_header;

Daniel

Re: [PATCH v4 3/6] linux/arm: account for COFF headers appearing at unexpected offsets

[Daniel Kiper]

On Thu, Sep 08, 2022 at 03:30:14PM +0200, 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 allows 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 <ardb@kernel.org>
> ---
>  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 7c0f17cf933d..56ba8d0a6ea3 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -63,6 +63,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);

I would check if grub_file_seek() does not return -1.

> +      if (grub_file_read (file, &lh->coff_image_header, sizeof(struct grub_coff_image_header))
> +         != sizeof(struct grub_coff_image_header))

Missing spaces before "("...

> +       return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF image header");

Please add missing spaces before return and after grub_error.

Daniel

Re: [PATCH v4 4/6] efi/efinet: Don't close connections at fini_hw() time

[Daniel Kiper]

On Thu, Sep 08, 2022 at 03:30:15PM +0200, Ard Biesheuvel wrote:
> When GRUB runs on top of EFI firmware, it only has access to block and
> network device abstractions exposed by the firmware, and it is up to the
> firmware to quiesce the underlying hardware when exiting boot services
> and handing over to the OS.
>
> This is especially important for network devices, to prevent incoming
> packets from being DMA'd straight into memory after the OS has taken
> over but before it has managed to reconfigure the network hardware.
>
> GRUB handles this by means of the grub_net_fini_hw() preboot hook, which
> is executed before calling into the booted image. This means that all
> network devices disappear or become inoperable before the EFI stub
> executes on EFI targeted builds. This is problematic as it prevents the
> EFI stub from calling back into GRUB provided protocols such as
> LoadFile2 for the initrd, which we will provide in a subsequent patch.
>
> So add a flag that indicates to the network core that EFI network
> devices should not be closed when grub_net_fini_hw() is called.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

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

Daniel

Re: [PATCH v4 5/6] efi: implement LoadFile2 initrd loading protocol for Linux

[Daniel Kiper]

On Thu, Sep 08, 2022 at 03:30:16PM +0200, Ard Biesheuvel wrote:
> Recent Linux kernels will invoke the LoadFile2 protocol installed on
> a well-known vendor media path 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 <ardb@kernel.org>
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  grub-core/commands/efi/lsefi.c |   1 +
>  grub-core/loader/arm64/linux.c | 123 +++++++++++++++++++-
>  include/grub/efi/api.h         |  40 +++++++
>  3 files changed, 163 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c
> index f46ba3b49384..c304d25ccdd6 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 file2" },
>      { 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/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 56ba8d0a6ea3..8b7fbb35fb72 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -48,6 +48,39 @@ 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 };

Please drop redundant spaces after "{" and before "}".

> +static grub_efi_handle_t initrd_lf2_handle = NULL;
> +static int initrd_use_loadfile2 = 0;

I think you can use bool here.

> +static grub_efi_guid_t load_file2_guid = GRUB_EFI_LOAD_FILE2_PROTOCOL_GUID;
> +static grub_efi_guid_t device_path_guid = GRUB_EFI_DEVICE_PATH_GUID;
> +
> +static initrd_media_device_path_t 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 __grub_efi_api
> +grub_efi_initrd_load_file2(grub_efi_load_file2_t *this,

Missing space before "(".

> +                           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 = {
> +  grub_efi_initrd_load_file2
> +};
> +
>  grub_err_t
>  grub_arch_efi_linux_load_image_header (grub_file_t file,
>                                        struct linux_arch_kernel_header * lh)
> @@ -78,6 +111,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;
>  }
>
> @@ -197,6 +242,8 @@ grub_linux_boot (void)
>  static grub_err_t
>  grub_linux_unload (void)
>  {
> +  grub_efi_boot_services_t *b = grub_efi_system_table->boot_services;
> +
>    grub_dl_unref (my_mod);
>    loaded = 0;
>    if (initrd_start)
> @@ -208,6 +255,18 @@ grub_linux_unload (void)
>      grub_efi_free_pages ((grub_addr_t) kernel_addr,
>  			 GRUB_EFI_BYTES_TO_PAGES (kernel_size));
>    grub_fdt_unload ();
> +
> +  if (initrd_lf2_handle)

initrd_lf2_handle != NULL

> +    {
> +      b->uninstall_multiple_protocol_interfaces (initrd_lf2_handle,
> +                                                 &load_file2_guid,
> +                                                 &initrd_lf2,
> +                                                 &device_path_guid,
> +                                                 &initrd_lf2_device_path,
> +                                                 NULL);
> +      initrd_lf2_handle = NULL;
> +      initrd_use_loadfile2 = 0;
> +    }
>    return GRUB_ERR_NONE;
>  }
>
> @@ -247,13 +306,50 @@ allocate_initrd_mem (int initrd_pages)
>  				       GRUB_EFI_LOADER_DATA);
>  }
>
> +static grub_efi_status_t __grub_efi_api
> +grub_efi_initrd_load_file2(grub_efi_load_file2_t *this,

Missing space before "(".

> +                          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 != &initrd_lf2 || buffer_size == NULL)
> +    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 == NULL || *buffer_size < initrd_size)
> +    {
> +      *buffer_size = initrd_size;
> +      return GRUB_EFI_BUFFER_TOO_SMALL;
> +    }
> +
> +  grub_dprintf ("linux", "Providing initrd via EFI_LOAD_FILE2_PROTOCOL\n");
> +
> +  if (grub_initrd_load (&initrd_ctx, buffer))
> +    status = GRUB_EFI_DEVICE_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_boot_services_t *b = grub_efi_system_table->boot_services;
> +  grub_efi_status_t status;
>
>    if (argc == 0)
>      {
> @@ -271,6 +367,31 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
>    if (grub_initrd_init (argc, argv, &initrd_ctx))
>      goto fail;
>
> +  if (initrd_use_loadfile2)
> +    {
> +      if (!initrd_lf2_handle)

initrd_lf2_handle = NULL

Daniel

Re: [PATCH v4 6/6] linux: ignore FDT unless we need to modify it

[Daniel Kiper]

On Thu, Sep 08, 2022 at 03:30:17PM +0200, Ard Biesheuvel wrote:
> 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 that when the LoadFile2 protocol is being
> used, there is no 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 <ardb@kernel.org>
> Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

Except three nits below Reviewed-by: Daniel Kiper <daniel.kiper@oracle.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 8b7fbb35fb72..36396e9f7ff5 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -133,21 +133,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 80d70887474a..fd8093ac83c9 100644
> --- a/grub-core/loader/efi/fdt.c
> +++ b/grub-core/loader/efi/fdt.c
> @@ -89,13 +89,16 @@ grub_fdt_install (void)
>    static grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
>    grub_efi_status_t status;
>
> +  if (!fdt && !loaded_fdt)

fdt == NULL && loaded_fdt == NULL

> +    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);

s/?:/? 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);

Ditto.

Daniel