[PATCH v2 0/2] EFI chainloader improvement

[PATCH v2 0/2] EFI chainloader improvement

[Glenn Washburn]

Changes since v1:
 * Rebase onto latest master
 * Change logic: The device path argument to image load changed back to the way
   it was originally where the argument is set to the device path that $root
   resolves to. If $root does not resolve or is not a device, the argument
   will be NULL as allowed for in the spec. By setting $root to the device of
   the chainloaded file, v1 behavior can be had. So this is more versatile
   behavior.
 * Minor rewording of metadata.

This series improves the EFI chainloader. I've noticed for a while now that
chainloading would fail when root=memdisk. It didn't really make sense because
I was specifying the image to chainload as device+path, so why would it care
about what my root was. But I noticed that if I changed the root to the device
the image file was located on, then chainloading worked. The second patch
fixes this by removing some previous assumptions that I don't believe are
valid (eg. that LoadImage needs a valid device path).

Glenn

Glenn Washburn (2):
  docs: Document extra arguments to chainloader on EFI
  efi/chainloader: Do not require a $root visible to EFI firmware when
    chainloading

 docs/grub.texi                     |  7 +++++--
 grub-core/loader/efi/chainloader.c | 28 +++++++++++++---------------
 2 files changed, 18 insertions(+), 17 deletions(-)

Range-diff against v1:
2:  04edeb1d7fd0 ! 1:  d1fc25518bfc docs: Document that extra arguments to chainloader on EFI
    @@ Metadata
     Author: Glenn Washburn <development@efficientek.com>
     
      ## Commit message ##
    -    docs: Document that extra arguments to chainloader on EFI
    +    docs: Document extra arguments to chainloader on EFI
     
         Extra arguments given to chainloader on EFI platforms will be sent to
         the chainloaded application. Also, minor edit in the chainloading section
    @@ Commit message
         necessarily in real mode (which does not exist on some achitectures).
     
      ## docs/grub.texi ##
    -@@ docs/grub.texi: information.
    +@@ docs/grub.texi: invoke shutdown machinery.
      Operating systems that do not support Multiboot and do not have specific
      support in GRUB (specific support is available for Linux, FreeBSD, NetBSD
      and OpenBSD) must be chain-loaded, which involves loading another boot
1:  eed74ee4b128 ! 2:  c17d3a8a0fe0 efi/chainloader: Do not require a valid $root when chainloading
    @@ Metadata
     Author: Glenn Washburn <development@efficientek.com>
     
      ## Commit message ##
    -    efi/chainloader: Do not require a valid $root when chainloading
    +    efi/chainloader: Do not require a $root visible to EFI firmware when chainloading
     
         The EFI chainloader checks that a device path can be created for the $root
         device before allowing chainloading to a given file. This is probably to
    @@ Commit message
         that is accessible by the firmware, eg. (hd0,1)/efi/boot.efi, would
         fail when root is a location inaccessible by the firmware, eg. memdisk.
     
    -    Use GRUB_EFI_BYTES_TO_PAGES() instead of donig the calculation explicitly.
    +    Use GRUB_EFI_BYTES_TO_PAGES() instead of doing the calculation explicitly.
     
         Add comment noting the section where the load options for the chainloaded
         EFI application is constructed.
    @@ grub-core/loader/efi/chainloader.c
      #include <grub/i18n.h>
      #include <grub/net.h>
     @@ grub-core/loader/efi/chainloader.c: grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
    -   if (! file)
    -     goto fail;
      
    --  /* Get the root device's device path.  */
    --  dev = grub_device_open (0);
    +   /* Get the root device's device path.  */
    +   dev = grub_device_open (0);
     -  if (! dev)
     -    goto fail;
     -
    -+  dev = file->device;
    -   if (dev->disk)
    +-  if (dev->disk)
    ++  if (dev == NULL)
    ++    ;
    ++  else if (dev->disk)
          dev_handle = grub_efidisk_get_device_handle (dev->disk);
        else if (dev->net && dev->net->server)
    +     {
     @@ grub-core/loader/efi/chainloader.c: grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
        if (dev_handle)
          dp = grub_efi_get_device_path (dev_handle);
    @@ grub-core/loader/efi/chainloader.c: grub_cmd_chainloader (grub_command_t cmd __a
     -  pages = (((grub_efi_uintn_t) size + ((1 << 12) - 1)) >> 12);
     +  pages = (grub_efi_uintn_t) GRUB_EFI_BYTES_TO_PAGES (size);
      
    -   status = efi_call_4 (b->allocate_pages, GRUB_EFI_ALLOCATE_ANY_PAGES,
    +   status = b->allocate_pages (GRUB_EFI_ALLOCATE_ANY_PAGES,
      			      GRUB_EFI_LOADER_CODE,
     @@ grub-core/loader/efi/chainloader.c: grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
          }
    @@ grub-core/loader/efi/chainloader.c: grub_cmd_chainloader (grub_command_t cmd __a
        if (argc > 1)
          {
            int i, len;
    -@@ grub-core/loader/efi/chainloader.c: grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
    -     }
    - 
    -   grub_file_close (file);
    --  grub_device_close (dev);
    - 
    -   /* We're finished with the source image buffer and file path now. */
    -   efi_call_2 (b->free_pages, address, pages);
    -@@ grub-core/loader/efi/chainloader.c: grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
    - 
    -  fail:
    - 
    --  if (dev)
    --    grub_device_close (dev);
    --
    -   if (file)
    -     grub_file_close (file);
    - 
-- 
2.34.1

[PATCH v2 1/2] docs: Document extra arguments to chainloader on EFI

[Glenn Washburn]

Extra arguments given to chainloader on EFI platforms will be sent to
the chainloaded application. Also, minor edit in the chainloading section
to note that chainloading can be a jump via the firmware and not
necessarily in real mode (which does not exist on some achitectures).

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 docs/grub.texi | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index e4c5517a6fc5..7ab1e0ab40ef 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -979,7 +979,7 @@ invoke shutdown machinery.
 Operating systems that do not support Multiboot and do not have specific
 support in GRUB (specific support is available for Linux, FreeBSD, NetBSD
 and OpenBSD) must be chain-loaded, which involves loading another boot
-loader and jumping to it in real mode.
+loader and jumping to it in real mode or via the firmware.
 
 The @command{chainloader} command (@pxref{chainloader}) is used to set this
 up.  It is normally also necessary to load some GRUB modules and set the
@@ -4031,10 +4031,13 @@ a list of commands that could use more documentation:
 @node chainloader
 @subsection chainloader
 
-@deffn Command chainloader [@option{--force}] file
+@deffn Command chainloader [@option{--force}] file [args...]
 Load @var{file} as a chain-loader. Like any other file loaded by the
 filesystem code, it can use the blocklist notation (@pxref{Block list
 syntax}) to grab the first sector of the current partition with @samp{+1}.
+On EFI platforms, any arguments after @var{file} will be sent to the loaded
+image.
+
 If you specify the option @option{--force}, then load @var{file} forcibly,
 whether it has a correct signature or not. This is required when you want to
 load a defective boot loader, such as SCO UnixWare 7.1.
-- 
2.34.1

[PATCH v2 2/2] efi/chainloader: Do not require a $root visible to EFI firmware when chainloading

[Glenn Washburn]

The EFI chainloader checks that a device path can be created for the $root
device before allowing chainloading to a given file. This is probably to
ensure that the given file can be accessed and loaded by the firmware.
However, since GRUB is loading the image itself, the firmware need not
be able to access the file location of the image. So remove this check.

Also, this fixes an issue where chainloading an image file on a location
that is accessible by the firmware, eg. (hd0,1)/efi/boot.efi, would
fail when root is a location inaccessible by the firmware, eg. memdisk.

Use GRUB_EFI_BYTES_TO_PAGES() instead of doing the calculation explicitly.

Add comment noting the section where the load options for the chainloaded
EFI application is constructed.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/loader/efi/chainloader.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index 1759af632fe9..1de98f783136 100644
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -32,6 +32,7 @@
 #include <grub/efi/api.h>
 #include <grub/efi/efi.h>
 #include <grub/efi/disk.h>
+#include <grub/efi/memory.h>
 #include <grub/command.h>
 #include <grub/i18n.h>
 #include <grub/net.h>
@@ -241,10 +242,9 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
 
   /* Get the root device's device path.  */
   dev = grub_device_open (0);
-  if (! dev)
-    goto fail;
-
-  if (dev->disk)
+  if (dev == NULL)
+    ;
+  else if (dev->disk)
     dev_handle = grub_efidisk_get_device_handle (dev->disk);
   else if (dev->net && dev->net->server)
     {
@@ -267,18 +267,15 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
   if (dev_handle)
     dp = grub_efi_get_device_path (dev_handle);
 
-  if (! dp)
+  if (dp != NULL)
     {
-      grub_error (GRUB_ERR_BAD_DEVICE, "not a valid root device");
-      goto fail;
-    }
-
-  file_path = make_file_path (dp, filename);
-  if (! file_path)
-    goto fail;
+      file_path = make_file_path (dp, filename);
+      if (file_path == NULL)
+	goto fail;
 
-  grub_printf ("file path: ");
-  grub_efi_print_device_path (file_path);
+      grub_printf ("file path: ");
+      grub_efi_print_device_path (file_path);
+    }
 
   size = grub_file_size (file);
   if (!size)
@@ -287,7 +284,7 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
 		  filename);
       goto fail;
     }
-  pages = (((grub_efi_uintn_t) size + ((1 << 12) - 1)) >> 12);
+  pages = (grub_efi_uintn_t) GRUB_EFI_BYTES_TO_PAGES (size);
 
   status = b->allocate_pages (GRUB_EFI_ALLOCATE_ANY_PAGES,
 			      GRUB_EFI_LOADER_CODE,
@@ -370,6 +367,7 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
     }
   loaded_image->device_handle = dev_handle;
 
+  /* Build load options with arguments from chainloader command line. */
   if (argc > 1)
     {
       int i, len;
-- 
2.34.1

Re: [PATCH v2 0/2] EFI chainloader improvement

[Ard Biesheuvel]

On Thu, 1 Jun 2023 at 06:17, Glenn Washburn <development@efficientek.com> wrote:
>
> Changes since v1:
>  * Rebase onto latest master
>  * Change logic: The device path argument to image load changed back to the way
>    it was originally where the argument is set to the device path that $root
>    resolves to. If $root does not resolve or is not a device, the argument
>    will be NULL as allowed for in the spec. By setting $root to the device of
>    the chainloaded file, v1 behavior can be had. So this is more versatile
>    behavior.
>  * Minor rewording of metadata.
>
> This series improves the EFI chainloader. I've noticed for a while now that
> chainloading would fail when root=memdisk. It didn't really make sense because
> I was specifying the image to chainload as device+path, so why would it care
> about what my root was. But I noticed that if I changed the root to the device
> the image file was located on, then chainloading worked. The second patch
> fixes this by removing some previous assumptions that I don't believe are
> valid (eg. that LoadImage needs a valid device path).
>

I don't think it ultimately matters that much what the device path is
when you provide the image via buffer+size, but I will note that the
generic EFI linux loader handles this case by constructing a 'memory
mapped' device path (in grub_arch_efi_linux_boot_image()), which just
describes a range of memory.

However, given that the device path in question is not installed on a
handle, I'm not even sure whether creating that memory mapped device
path serves any purpose tbh.

So this approach seems perfectly reasonable to me.

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Re: [PATCH v2 0/2] EFI chainloader improvement

[Daniel Kiper]

On Thu, Jun 01, 2023 at 09:33:25AM +0200, Ard Biesheuvel wrote:
> On Thu, 1 Jun 2023 at 06:17, Glenn Washburn <development@efficientek.com> wrote:
> >
> > Changes since v1:
> >  * Rebase onto latest master
> >  * Change logic: The device path argument to image load changed back to the way
> >    it was originally where the argument is set to the device path that $root
> >    resolves to. If $root does not resolve or is not a device, the argument
> >    will be NULL as allowed for in the spec. By setting $root to the device of
> >    the chainloaded file, v1 behavior can be had. So this is more versatile
> >    behavior.
> >  * Minor rewording of metadata.
> >
> > This series improves the EFI chainloader. I've noticed for a while now that
> > chainloading would fail when root=memdisk. It didn't really make sense because
> > I was specifying the image to chainload as device+path, so why would it care
> > about what my root was. But I noticed that if I changed the root to the device
> > the image file was located on, then chainloading worked. The second patch
> > fixes this by removing some previous assumptions that I don't believe are
> > valid (eg. that LoadImage needs a valid device path).
> >
>
> I don't think it ultimately matters that much what the device path is
> when you provide the image via buffer+size, but I will note that the
> generic EFI linux loader handles this case by constructing a 'memory
> mapped' device path (in grub_arch_efi_linux_boot_image()), which just
> describes a range of memory.
>
> However, given that the device path in question is not installed on a
> handle, I'm not even sure whether creating that memory mapped device
> path serves any purpose tbh.
>
> So this approach seems perfectly reasonable to me.
>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

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

Thank you for fixing this issue!

Daniel