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