On Wed, Oct 30, 2024 at 05:12:48PM GMT, Daniel Kiper wrote:
> Adding Leo...
>
> On Tue, Oct 29, 2024 at 03:57:18PM +0800, Michael Chang via Grub-devel wrote:
> > The "cmdpath" environment variable is set at startup to the location
> > from which the grub image is loaded. It includes a device part and,
> > optionally, an absolute directory name if the grub image is booted as a
> > file in a local file-system directory, or in a remote server directory,
> > like TFTP.
> >
> > This entire process relies on firmware to provide the correct device
> > path of the booted image.
> >
> > We encountered an issue when the image is booted from the root
> > directory, where the absolute directory name "/" is discarded. This
> > makes it unclear whether the root path was missing in the firmware
> > provided device path or if it is simply the root directory. This
> > ambiguity can cause confusion in custom scripts, potentially causing
> > them to interpret firmware data incorrectly and trigger unintended
> > fallback measures.
> >
> > This patch fixes the problem by properly assigning the "fwpath" returned
> > by "grub_machine_get_bootlocation()" to "cmdpath". The fix is based on
> > the fact that fwpath is NULL if the firmware didn’t provide a path part
> > or an NUL character, "", if it represents the root directory. With this,
> > it becomes possible to clearly distinguish:
> >
> > - cmdpath=(hd0,1) - Either the image is booted from the first (raw)
> > partition, or the firmware failed to provide the path part.
> > - cmdpath=(hd0,1)/ - The image is booted from the root directory in the
> > first partition.
> >
> > As a side note, the fix is similar to [1], but without the renaming
> > part.
> >
> > [1] https://mail.gnu.org/archive/html/grub-devel/2024-10/msg00155.html
> >
> > Signed-off-by: Michael Chang <mch...@suse.com>
> > ---
> > grub-core/kern/main.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> > index d29494d54..6645173c1 100644
> > --- a/grub-core/kern/main.c
> > +++ b/grub-core/kern/main.c
> > @@ -136,7 +136,11 @@ grub_set_prefix_and_root (void)
> > {
> > char *cmdpath;
> >
> > - cmdpath = grub_xasprintf ("(%s)%s", fwdevice, fwpath ? : "");
> > + if (fwpath && *fwpath == '\0')
> > + cmdpath = grub_xasprintf ("(%s)/", fwdevice);
> > + else
> > + cmdpath = grub_xasprintf ("(%s)%s", fwdevice, fwpath ? : "");
>
> Would not this below work?
>
> cmdpath = grub_xasprintf ("(%s)%s", fwdevice, (fwpath == NULL || *fwpath ==
> '\0') ? "/" : fwpath);
It looks to me that this expression sets the path part of cmdpath to "/"
when fwpath == NULL. This is not desired, as it could mislead people
into thinking grub is booted from a root directory, when in fact it
might be a raw partition or disk.
>
> And if yes does this (completely) replace the patch mentioned by you above?
The mentioned patch is actually more complicated. I think it could be
broken down into three parts:
1. Fixing the boot from the root directory issue, as this patch does.
2. Renaming cmdpath to fw_path.
3. Using the new fw_path to look up grub.cfg.
My concerns with respect to these three parts are:
1. Although it fixes the root directory issue, it also intentionally
ignores the case where fwpath is set to NULL. IMHO this is problematic
because booting from a raw partition or disk is valid, many setups such
as MBR, GPT BIOS Boot, and PPC PReP, are still actively used today.
2. Renaming the well-known cmdpath may break custom (third-party) scripts.
3. There is already a mechanism for looking up grub.cfg using the prefix
variable. If prefix is not set (e.g., when grub-mkimage is used without
specifying -p ..), its value is derived automatically from
fwdevice/fwpath. This makes the change seem redundant, as it’s already
covered.
So, unless these concerns can be clarified, I think this simpler patch
makes sense, as it addresses one problem at a time in an obvious way.
>
> Last but not least, please next time CC original author of the patch you
> are referring to.
Thanks for the reminder. I will.
Michael
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
