[PATCH 0/4] linux: implement LoadFile2 initrd loading

[PATCH 0/4] 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

Ard Biesheuvel (4):
  loader/linux: permit NULL argument for argv[] in grub_initrd_load()
  efi: add definition of LoadFile2 protocol
  efi: implemented LoadFile2 initr loading protocol for Linux
  linux: ignore FDT unless we need to modify it

 grub-core/commands/efi/lsefi.c |   1 +
 grub-core/loader/arm64/linux.c | 139 ++++++++++++++++++--
 grub-core/loader/efi/fdt.c     |   7 +-
 grub-core/loader/linux.c       |   2 +-
 include/grub/efi/api.h         |  15 +++
 5 files changed, 149 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH 1/4] 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>
---
 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 2/4] 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>
---
 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 3/4] efi: implemented LoadFile2 initr 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 <ard.biesheuvel@arm.com>
---
 grub-core/loader/arm64/linux.c | 117 +++++++++++++++++++-
 1 file changed, 116 insertions(+), 1 deletion(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index ef3e9f9444ca..285422c7bd43 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -48,9 +48,16 @@ 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_check_image (struct linux_arch_kernel_header * lh)
 {
+  struct grub_pe32_coff_header *coff_header;
+  struct grub_pe32_optional_header *optional_header;
+
   if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE)
     return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
 
@@ -61,6 +68,21 @@ grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh)
   grub_dprintf ("linux", "UEFI stub kernel:\n");
   grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
 
+  coff_header = (struct grub_pe32_coff_header *)((unsigned long)lh + lh->hdr_offset);
+  optional_header = (struct grub_pe32_optional_header *)(coff_header + 1);
+
+  /*
+   * Linux kernels built for any architecture are guaranteed to support the
+   * LoadFile2 based initrd loading protocol if the image version is >= 1.
+   */
+  if (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;
 }
 
@@ -230,13 +252,88 @@ 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
+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
+};
+
+static grub_efi_status_t
+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 || 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)
     {
@@ -254,6 +351,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
   if (grub_initrd_init (argc, argv, &initrd_ctx))
     goto fail;
 
+  if (initrd_use_loadfile2 && !initrd_lf2_handle)
+    {
+      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 4/4] linux: ignore FDT unless we need to modify it

[Ard Biesheuvel]

Now that we implemented supported 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>
---
 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 285422c7bd43..9e282b6660fe 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -93,21 +93,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 3/4] efi: implemented LoadFile2 initr loading protocol for Linux

[Leif Lindholm]

initrd typo in subject

minor style comments below

On Fri, Oct 23, 2020 at 14:08:24 +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 <ard.biesheuvel@arm.com>
> ---
>  grub-core/loader/arm64/linux.c | 117 +++++++++++++++++++-
>  1 file changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index ef3e9f9444ca..285422c7bd43 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -48,9 +48,16 @@ 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_check_image (struct linux_arch_kernel_header * lh)
>  {
> +  struct grub_pe32_coff_header *coff_header;
> +  struct grub_pe32_optional_header *optional_header;
> +
>    if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE)
>      return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
>  
> @@ -61,6 +68,21 @@ grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh)
>    grub_dprintf ("linux", "UEFI stub kernel:\n");
>    grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
>  
> +  coff_header = (struct grub_pe32_coff_header *)((unsigned long)lh + lh->hdr_offset);
> +  optional_header = (struct grub_pe32_optional_header *)(coff_header + 1);
> +
> +  /*
> +   * Linux kernels built for any architecture are guaranteed to support the
> +   * LoadFile2 based initrd loading protocol if the image version is >= 1.
> +   */
> +  if (optional_header->major_image_version >= 1)
> +    initrd_use_loadfile2 = 1;
> +   else

funky indentation

> +    initrd_use_loadfile2 = 0;
> +
> +  grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n",
> +		initrd_use_loadfile2 ? "en" : "dis");
> +
>    return GRUB_ERR_NONE;
>  }
>  
> @@ -230,13 +252,88 @@ 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
> +grub_efi_initrd_load_file2(grub_efi_load_file2_t *this,

Static functions don't need global prefixes (i.e., initrd_load_file2
would be sufficient).

> +                           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
> +};
> +
> +static grub_efi_status_t
> +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 || 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)
>      {
> @@ -254,6 +351,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
>    if (grub_initrd_init (argc, argv, &initrd_ctx))
>      goto fail;
>  
> +  if (initrd_use_loadfile2 && !initrd_lf2_handle)
> +    {
> +      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)
> +        {

indented by spaces

> +	  grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> +	  return grub_errno;
> +	}

indented with tab

/
    Leif

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

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

[Leif Lindholm]

On Fri, Oct 23, 2020 at 14:08:25 +0200, Ard Biesheuvel wrote:
> Now that we implemented supported 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.

There is a change in behaviour here which I don't think matters, but
I'll call it out anyway:
If there was ever a kernel out there with an EFI stub that depended on
a chosen node existing in the DT, and the one provide by firmware did
not contain one, that setup would break from this *if* it didn't use
an initrd.

/
    Leif

> 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>
> ---
>  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 285422c7bd43..9e282b6660fe 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -93,21 +93,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 0/4] linux: implement LoadFile2 initrd loading

[Leif Lindholm]

On Fri, Oct 23, 2020 at 14:08:21 +0200, Ard Biesheuvel wrote:
> 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]

I don't review enough grub code to be certain I've caught all aspects
of style adherence, so with that in mind, for 1-2/4:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

For 3-4/4, I did have some minor comments, but this is a really great
feature and I would like to see it merged.

/
    Leif

> Cc: grub-devel@gnu.org
> Cc: daniel.kiper@oracle.com
> Cc: leif@nuviainc.com
> 
> Ard Biesheuvel (4):
>   loader/linux: permit NULL argument for argv[] in grub_initrd_load()
>   efi: add definition of LoadFile2 protocol
>   efi: implemented LoadFile2 initr loading protocol for Linux
>   linux: ignore FDT unless we need to modify it
> 
>  grub-core/commands/efi/lsefi.c |   1 +
>  grub-core/loader/arm64/linux.c | 139 ++++++++++++++++++--
>  grub-core/loader/efi/fdt.c     |   7 +-
>  grub-core/loader/linux.c       |   2 +-
>  include/grub/efi/api.h         |  15 +++
>  5 files changed, 149 insertions(+), 15 deletions(-)
> 
> -- 
> 2.17.1
> 

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

[Ard Biesheuvel]

On Fri, 23 Oct 2020 at 14:47, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Fri, Oct 23, 2020 at 14:08:25 +0200, Ard Biesheuvel wrote:
> > Now that we implemented supported 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.
>
> There is a change in behaviour here which I don't think matters, but
> I'll call it out anyway:
> If there was ever a kernel out there with an EFI stub that depended on
> a chosen node existing in the DT, and the one provide by firmware did
> not contain one, that setup would break from this *if* it didn't use
> an initrd.
>

I checked the Linux source, and the original code contributed by Roy
already contained the logic to create the /chosen node if it wants
there already. So we should be fine here.

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

[Leif Lindholm]

On Fri, Oct 23, 2020 at 15:12:50 +0200, Ard Biesheuvel wrote:
> On Fri, 23 Oct 2020 at 14:47, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > On Fri, Oct 23, 2020 at 14:08:25 +0200, Ard Biesheuvel wrote:
> > > Now that we implemented supported 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.
> >
> > There is a change in behaviour here which I don't think matters, but
> > I'll call it out anyway:
> > If there was ever a kernel out there with an EFI stub that depended on
> > a chosen node existing in the DT, and the one provide by firmware did
> > not contain one, that setup would break from this *if* it didn't use
> > an initrd.
> 
> I checked the Linux source, and the original code contributed by Roy
> already contained the logic to create the /chosen node if it wants
> there already. So we should be fine here.

Excellent. Then, with this information now in a public archive for any
unfortunate souls doing anything crazy non-linux to find:

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

Re: [PATCH 3/4] efi: implemented LoadFile2 initr loading protocol for Linux

[Ard Biesheuvel]

On Fri, 23 Oct 2020 at 14:32, Leif Lindholm <leif@nuviainc.com> wrote:
>
> initrd typo in subject
>
> minor style comments below
>
> On Fri, Oct 23, 2020 at 14:08:24 +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 <ard.biesheuvel@arm.com>
> > ---
> >  grub-core/loader/arm64/linux.c | 117 +++++++++++++++++++-
> >  1 file changed, 116 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > index ef3e9f9444ca..285422c7bd43 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -48,9 +48,16 @@ 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_check_image (struct linux_arch_kernel_header * lh)
> >  {
> > +  struct grub_pe32_coff_header *coff_header;
> > +  struct grub_pe32_optional_header *optional_header;
> > +
> >    if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE)
> >      return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
> >
> > @@ -61,6 +68,21 @@ grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh)
> >    grub_dprintf ("linux", "UEFI stub kernel:\n");
> >    grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
> >
> > +  coff_header = (struct grub_pe32_coff_header *)((unsigned long)lh + lh->hdr_offset);
> > +  optional_header = (struct grub_pe32_optional_header *)(coff_header + 1);
> > +
> > +  /*
> > +   * Linux kernels built for any architecture are guaranteed to support the
> > +   * LoadFile2 based initrd loading protocol if the image version is >= 1.
> > +   */
> > +  if (optional_header->major_image_version >= 1)
> > +    initrd_use_loadfile2 = 1;
> > +   else
>
> funky indentation
>
> > +    initrd_use_loadfile2 = 0;
> > +
> > +  grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n",
> > +             initrd_use_loadfile2 ? "en" : "dis");
> > +
> >    return GRUB_ERR_NONE;
> >  }
> >
> > @@ -230,13 +252,88 @@ 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
> > +grub_efi_initrd_load_file2(grub_efi_load_file2_t *this,
>
> Static functions don't need global prefixes (i.e., initrd_load_file2
> would be sufficient).
>
> > +                           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
> > +};
> > +
> > +static grub_efi_status_t
> > +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 || 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)
> >      {
> > @@ -254,6 +351,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
> >    if (grub_initrd_init (argc, argv, &initrd_ctx))
> >      goto fail;
> >
> > +  if (initrd_use_loadfile2 && !initrd_lf2_handle)
> > +    {
> > +      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)
> > +        {
>
> indented by spaces
>
> > +       grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> > +       return grub_errno;
> > +     }
>
> indented with tab
>

Thanks. I'll fix that up for the next revision. There's actually a bug
in this patch which I need to fix too: the code that checks the
PE/COFF header for the image version doesn't actually load the entire
header :-(