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 <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.
>
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
