On Fri, 26 Aug 2022 10:12:18 +0100 Dimitri John Ledkov <dimitri.led...@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 > <developm...@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 <developm...@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 > > > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
