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

[Michael Chang]

On Mon, Nov 04, 2024 at 07:15:33PM GMT, Daniel Kiper wrote:
> 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.

OK. I will add it in next revision.

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

I was referring to this hunk in the patch "Add fw_path variable":    

 diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
 index d3f53d93d..08f48c71d 100644
 --- a/grub-core/normal/main.c
 +++ b/grub-core/normal/main.c
 @@ -339,7 +339,30 @@ grub_cmd_normal (struct grub_command *cmd __attribute__ 
((unused)),
        /* Guess the config filename. It is necessary to make CONFIG static,
         so that it won't get broken by longjmp.  */
        char *config;
 -      const char *prefix;
 +      const char *prefix, *fw_path;
 +
 +      fw_path = grub_env_get ("fw_path");
 +      if (fw_path)
 +      {
 +        config = grub_xasprintf ("%s/grub.cfg", fw_path);
 +        if (config)
 +          {
 +            grub_file_t file;
 +
 +            file = grub_file_open (config, GRUB_FILE_TYPE_CONFIG);
 +            if (file)
 +              {
 +                grub_file_close (file);
 +                grub_enter_normal_mode (config);
 +              }
 +              else
 +                {
 +                  /*  Ignore all errors.  */
 +                  grub_errno = 0;
 +                }
 +            grub_free (config);
 +          }
 +      }
  
        prefix = grub_env_get ("prefix");
        if (prefix)

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

Let's check with Leo first for his feedback to this patch.

Hi Leo,

What's your take on this ? Could you give this patch a go, or not? If not, why?

Thanks,
Michael
> 
> > > 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