Re: [PATCH] kern/main: Fix cmdpath in root directory

[Daniel Kiper]

On Fri, Nov 01, 2024 at 10:03:16AM +0800, Michael Chang wrote:
> 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 <mchang@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.

OK, then if/else requires a comment describing what is happening there.

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

OK...

> 2. Renaming the well-known cmdpath may break custom (third-party) scripts.

This is not acceptable. So, cmdpath variable name cannot be changed.
And I suggest to not change its name in downstream too.

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

Which change are you referring to?

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

Michael, Leo, please agree how you want to proceed further taking into
account above.

> > Last but not least, please next time CC original author of the patch you
> > are referring to.
>
> Thanks for the reminder. I will.

Thanks!

Daniel