[PATCH 0/2] EFI chainloader improvement

[PATCH 0/2] EFI chainloader improvement

[Glenn Washburn]

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):
  efi/chainloader: Do not require a valid $root when chainloading
  docs: Document that extra arguments to chainloader on EFI

 docs/grub.texi                     |  7 +++++--
 grub-core/loader/efi/chainloader.c | 31 +++++++++++-------------------
 2 files changed, 16 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH 1/2] efi/chainloader: Do not require a valid $root 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 donig 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 | 31 +++++++++++-------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index 7557eb269b..5aac3c59fd 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>
@@ -239,11 +240,7 @@ 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);
-  if (! dev)
-    goto fail;
-
+  dev = file->device;
   if (dev->disk)
     dev_handle = grub_efidisk_get_device_handle (dev->disk);
   else if (dev->net && dev->net->server)
@@ -267,18 +264,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 +281,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 = efi_call_4 (b->allocate_pages, GRUB_EFI_ALLOCATE_ANY_PAGES,
 			      GRUB_EFI_LOADER_CODE,
@@ -370,6 +364,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;
@@ -400,7 +395,6 @@ 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);
@@ -411,9 +405,6 @@ 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 2/2] docs: Document that 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 a463f9e354..34a00dd5a9 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -965,7 +965,7 @@ information.
 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
@@ -4011,10 +4011,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

Re: [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading

[Dimitri John Ledkov]

Hi,

This is interesting. I had to work around this same issue in loopback
to allow chainloading from loopback devices see
https://github.com/rhboot/grub2/commit/0e5cb733f3cb227293ea58397ea10891519095f0



On Fri, 26 Aug 2022 at 05:34, Glenn Washburn
<development@efficientek.com> wrote:
>
> 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 donig 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 | 31 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
>
> diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
> index 7557eb269b..5aac3c59fd 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>
> @@ -239,11 +240,7 @@ 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);
> -  if (! dev)
> -    goto fail;
> -
> +  dev = file->device;
>    if (dev->disk)
>      dev_handle = grub_efidisk_get_device_handle (dev->disk);
>    else if (dev->net && dev->net->server)
> @@ -267,18 +264,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 +281,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 = efi_call_4 (b->allocate_pages, GRUB_EFI_ALLOCATE_ANY_PAGES,
>                               GRUB_EFI_LOADER_CODE,
> @@ -370,6 +364,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;
> @@ -400,7 +395,6 @@ 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);
> @@ -411,9 +405,6 @@ 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
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
okurrr,

Dimitri

Re: [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading

[Glenn Washburn]

On Fri, 26 Aug 2022 10:12:18 +0100
Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:

> Hi,
> 
> This is interesting. I had to work around this same issue in loopback
> to allow chainloading from loopback devices see
> https://github.com/rhboot/grub2/commit/0e5cb733f3cb227293ea58397ea10891519095f0

Hmm, now that I think about this more, the current code is in some way
more general. For instance, here's how you can get around your loopback
issue. Suppose you have a loopback device, (d), backed by a file,
(hd0,1)/fat.img, with an efi application at /efi/app.efi and you want to
chainload that. To achieve the same thing as your patch, but with
current code you would do:

  root=(hd0,1)
  chainloader (d)/efi/app.efi

So the current code allows settings the device_handle independently of
where the chainloaded file is residing. Its annoying for me when my
root is a memdisk, which forces me to change root. I'm guessing you
have your root set to the loopback device, which is what caused you
headaches. My question for the EFI gurus or anyone who might have
experience with the expectation of other EFI apps is: How are EFI
apps in the wild known to use the device_handle of their loaded image?
Are there any that do bad things when it is NULL? And if so, do we
really care about them? And how might apps do something undesirable if
passed a device_handle not associated with where they were actually
loaded from? As I'm seeing it, the device_handle is meant to be
basically like a handle to CWD.

So now I'm thinking that perhaps a better way to do this to preserve
functionality with existing code is to add a "-d" option to chainloader
that where the device_handle can be passed implicitly. The new way to
do the above would be like this:

  chainloader -d (hd0,1) (d)/efi/app.efi

The default behavior should be what the end user most likely would
want. So I'm thinking that not providing a -d would try to get the
device handle from the chainloaded file. If that fails use root, and if
that fails provide a NULL device_handle. Or should we not do fallbacks
and just have NULL device_handle if getting a device path for the file
fails?

From my perspective as a user (before understanding the issue more), it
didn't make sense that I should have to set root to chainload a file on
a device, partition, and filesystem that the firmware natively
understood. So I don't think chainloader should have this behavior to
avoid confusion (though thankfully there is an error message that clued
me in to how to resolve the issue).

Glenn

> 
> On Fri, 26 Aug 2022 at 05:34, Glenn Washburn
> <development@efficientek.com> wrote:
> >
> > 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 donig 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 | 31 +++++++++++-------------------
> >  1 file changed, 11 insertions(+), 20 deletions(-)
> >
> > diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
> > index 7557eb269b..5aac3c59fd 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>
> > @@ -239,11 +240,7 @@ 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);
> > -  if (! dev)
> > -    goto fail;
> > -
> > +  dev = file->device;
> >    if (dev->disk)
> >      dev_handle = grub_efidisk_get_device_handle (dev->disk);
> >    else if (dev->net && dev->net->server)
> > @@ -267,18 +264,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 +281,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 = efi_call_4 (b->allocate_pages, GRUB_EFI_ALLOCATE_ANY_PAGES,
> >                               GRUB_EFI_LOADER_CODE,
> > @@ -370,6 +364,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;
> > @@ -400,7 +395,6 @@ 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);
> > @@ -411,9 +405,6 @@ 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
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> 

Re: [PATCH 0/2] EFI chainloader improvement

[Glenn Washburn]

On Thu, 25 Aug 2022 23:32:01 -0500
Glenn Washburn <development@efficientek.com> wrote:

> 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).

In a previous response to Dimitri Ledkov, I got confused as to what the
DevicePath parameter to BootService.LoadImage was supposed to be. I
was thinking it could be a kind of equivalent to the shell's current
working directory. According to the spec 2.10 7.4.1, it is "the
DeviceHandle specific file path from which the image is loaded." So
really $root should have nothing to do with it, unless the chainloaded
file is not specified with a leading device component. My conclusion is
that the current implementation violates the spec.

The right thing to do is to try to make a device path from the path of
the chainloaded file. If a device path can not be made (eg. the file
can not be accessed from the firmware) then the DevicePath parameter
should be NULL, which is valid because the parameter is marked optional.

So, Daniel, actually I think this patch does the right thing and is
ready for review.

Glenn

> Glenn Washburn (2):
>   efi/chainloader: Do not require a valid $root when chainloading
>   docs: Document that extra arguments to chainloader on EFI
> 
>  docs/grub.texi                     |  7 +++++--
>  grub-core/loader/efi/chainloader.c | 31
> +++++++++++------------------- 2 files changed, 16 insertions(+), 22
> deletions(-)
> 

Re: [PATCH 0/2] EFI chainloader improvement

[Daniel Kiper]

On Thu, Aug 25, 2022 at 11:32:01PM -0500, Glenn Washburn wrote:
> 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):
>   efi/chainloader: Do not require a valid $root when chainloading
>   docs: Document that extra arguments to chainloader on EFI

The patch set LGTM. Sadly it does not apply due to significant changes
in the EFI code. Could you rebase it on top of [1].

Daniel

[1] https://lists.gnu.org/archive/html/grub-devel/2023-05/msg00147.html