* [PATCH 0/2] EFI chainloader improvement
@ 2022-08-26 4:32 Glenn Washburn
2022-08-26 4:32 ` [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading Glenn Washburn
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Glenn Washburn @ 2022-08-26 4:32 UTC (permalink / raw)
To: grub-devel, Daniel Kiper; +Cc: Ard Biesheuvel, 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading 2022-08-26 4:32 [PATCH 0/2] EFI chainloader improvement Glenn Washburn @ 2022-08-26 4:32 ` Glenn Washburn 2022-08-26 9:12 ` Dimitri John Ledkov 2022-08-26 4:32 ` [PATCH 2/2] docs: Document that extra arguments to chainloader on EFI Glenn Washburn ` (2 subsequent siblings) 3 siblings, 1 reply; 7+ messages in thread From: Glenn Washburn @ 2022-08-26 4:32 UTC (permalink / raw) To: grub-devel, Daniel Kiper; +Cc: Ard Biesheuvel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading 2022-08-26 4:32 ` [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading Glenn Washburn @ 2022-08-26 9:12 ` Dimitri John Ledkov 2022-08-26 18:02 ` Glenn Washburn 0 siblings, 1 reply; 7+ messages in thread From: Dimitri John Ledkov @ 2022-08-26 9:12 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Daniel Kiper, Ard Biesheuvel, Glenn Washburn 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading 2022-08-26 9:12 ` Dimitri John Ledkov @ 2022-08-26 18:02 ` Glenn Washburn 0 siblings, 0 replies; 7+ messages in thread From: Glenn Washburn @ 2022-08-26 18:02 UTC (permalink / raw) To: Dimitri John Ledkov Cc: The development of GNU GRUB, Daniel Kiper, Ard Biesheuvel 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] docs: Document that extra arguments to chainloader on EFI 2022-08-26 4:32 [PATCH 0/2] EFI chainloader improvement Glenn Washburn 2022-08-26 4:32 ` [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading Glenn Washburn @ 2022-08-26 4:32 ` Glenn Washburn 2022-12-15 21:45 ` [PATCH 0/2] EFI chainloader improvement Glenn Washburn 2023-05-30 14:59 ` Daniel Kiper 3 siblings, 0 replies; 7+ messages in thread From: Glenn Washburn @ 2022-08-26 4:32 UTC (permalink / raw) To: grub-devel, Daniel Kiper; +Cc: Ard Biesheuvel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] EFI chainloader improvement 2022-08-26 4:32 [PATCH 0/2] EFI chainloader improvement Glenn Washburn 2022-08-26 4:32 ` [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading Glenn Washburn 2022-08-26 4:32 ` [PATCH 2/2] docs: Document that extra arguments to chainloader on EFI Glenn Washburn @ 2022-12-15 21:45 ` Glenn Washburn 2023-05-30 14:59 ` Daniel Kiper 3 siblings, 0 replies; 7+ messages in thread From: Glenn Washburn @ 2022-12-15 21:45 UTC (permalink / raw) To: grub-devel, Daniel Kiper; +Cc: Ard Biesheuvel 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(-) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] EFI chainloader improvement 2022-08-26 4:32 [PATCH 0/2] EFI chainloader improvement Glenn Washburn ` (2 preceding siblings ...) 2022-12-15 21:45 ` [PATCH 0/2] EFI chainloader improvement Glenn Washburn @ 2023-05-30 14:59 ` Daniel Kiper 3 siblings, 0 replies; 7+ messages in thread From: Daniel Kiper @ 2023-05-30 14:59 UTC (permalink / raw) To: Glenn Washburn; +Cc: grub-devel, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-30 15:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-26 4:32 [PATCH 0/2] EFI chainloader improvement Glenn Washburn 2022-08-26 4:32 ` [PATCH 1/2] efi/chainloader: Do not require a valid $root when chainloading Glenn Washburn 2022-08-26 9:12 ` Dimitri John Ledkov 2022-08-26 18:02 ` Glenn Washburn 2022-08-26 4:32 ` [PATCH 2/2] docs: Document that extra arguments to chainloader on EFI Glenn Washburn 2022-12-15 21:45 ` [PATCH 0/2] EFI chainloader improvement Glenn Washburn 2023-05-30 14:59 ` Daniel Kiper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.