Re: [PATCH v2 1/9] util/grub-editenv: add basic structures and probe call for external envblk

[Avnish Chouhan <avnish@linux.ibm.com>]

On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
> 
> Message: 2
> Date: Mon, 15 Sep 2025 17:08:40 +0800
> From: Michael Chang <mchang@suse.com>
> To: The development of GNU GRUB <grub-devel@gnu.org>
> Cc: Neal Gompa <ngompa13@gmail.com>,	Marta Lewandowska
> 	<mlewando@redhat.com>
> Subject: [PATCH v2 1/9] util/grub-editenv: add basic structures and
> 	probe call for external envblk
> Message-ID: <20250915090848.131937-2-mchang@suse.com>
> 
> This patch prepares for using an environment block stored in a reserved
> area of the filesystem. It adds a constant ENV_BTRFS_OFFSET at 256 KiB
> in the Btrfs header. It also introduces the fs_envblk_spec and 
> fs_envblk
> structures together with the probe_fs_envblk function to identify the
> root filesystem and to avoid configurations that involve diskfilter or
> cryptodisk.
> 
> The probe is only invoked when grub-editenv is working on the default
> environment file path. This restriction ensures that probing and
> possible raw device access are not triggered for arbitrary user 
> supplied
> paths, but only for the standard grubenv file. In that case the code
> checks if the filename equals DEFAULT_ENVBLK_PATH and then calls
> probe_fs_envblk with fs_envblk_spec. The result is stored in the global
> fs_envblk handle. At this stage the external environment block is only
> detected and recorded, and the behavior of grub editenv is unchanged.
> 
> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>  include/grub/fs.h   |   2 +
>  util/grub-editenv.c | 153 +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 154 insertions(+), 1 deletion(-)
> 
> diff --git a/include/grub/fs.h b/include/grub/fs.h
> index df4c93b16..1be26dfba 100644
> --- a/include/grub/fs.h
> +++ b/include/grub/fs.h
> @@ -132,4 +132,6 @@ grub_fs_unregister (grub_fs_t fs)
> 
>  grub_fs_t EXPORT_FUNC(grub_fs_probe) (grub_device_t device);
> 
> +#define ENV_BTRFS_OFFSET (256 * 1024)
> +
>  #endif /* ! GRUB_FS_HEADER */
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index db6f187cc..2302a6acf 100644
> --- a/util/grub-editenv.c
> +++ b/util/grub-editenv.c
> @@ -23,8 +23,11 @@
>  #include <grub/util/misc.h>
>  #include <grub/lib/envblk.h>
>  #include <grub/i18n.h>
> -#include <grub/emu/hostfile.h>
> +#include <grub/emu/hostdisk.h>
>  #include <grub/util/install.h>
> +#include <grub/emu/getroot.h>
> +#include <grub/fs.h>
> +#include <grub/crypto.h>
> 
>  #include <stdio.h>
>  #include <unistd.h>
> @@ -120,6 +123,26 @@ block, use `rm %s'."),
>    NULL, help_filter, NULL
>  };
> 
> +struct fs_envblk_spec {

Hi Michael,

This '{' seems to be in a new line!

> +  const char *fs_name;
> +  off_t offset;
> +  size_t size;
> +};
> +typedef struct fs_envblk_spec *fs_envblk_spec_t;
> +
> +struct fs_envblk {

Same as above!

> +  struct fs_envblk_spec *spec;
> +  const char *dev;
> +};
> +typedef struct fs_envblk *fs_envblk_t;
> +
> +static struct fs_envblk_spec fs_envblk_spec[] = {
> +  { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE },
> +  { NULL, 0, 0 }
> +};
> +
> +static fs_envblk_t fs_envblk = NULL;
> +
>  static grub_envblk_t
>  open_envblk_file (const char *name)
>  {
> @@ -253,6 +276,131 @@ unset_variables (const char *name, int argc, char 
> *argv[])
>    grub_envblk_close (envblk);
>  }
> 
> +static bool
> +is_abstraction (grub_device_t dev)
> +{
> +  if (dev == NULL || dev->disk == NULL)
> +    return false;
> +
> +  if (dev->disk->dev->id == GRUB_DISK_DEVICE_DISKFILTER_ID ||
> +      dev->disk->dev->id == GRUB_DISK_DEVICE_CRYPTODISK_ID)
> +    return true;

Adding an empty line would be better here!

> +  return false;
> +}
> +
> +static fs_envblk_t
> +probe_fs_envblk (fs_envblk_spec_t spec)
> +{
> +  char **grub_devices = NULL;
> +  char **curdev, **curdrive;
> +  size_t ndev = 0;
> +  char **grub_drives = NULL;
> +  grub_device_t grub_dev = NULL;
> +  grub_fs_t grub_fs = NULL;
> +  char *devname = NULL;
> +  fs_envblk_spec_t p;
> +  bool have_abstraction = false;
> +
> +  grub_util_biosdisk_init (DEFAULT_DEVICE_MAP);
> +  grub_init_all ();
> +  grub_gcry_init_all ();
> +
> +  grub_lvm_fini ();
> +  grub_mdraid09_fini ();
> +  grub_mdraid1x_fini ();
> +  grub_diskfilter_fini ();
> +  grub_diskfilter_init ();
> +  grub_mdraid09_init ();
> +  grub_mdraid1x_init ();
> +  grub_lvm_init ();

We aren't checking any failure in these initializes? Are we good without 
the checks here.. I'm not sure though!

> +
> +  grub_devices = grub_guess_root_devices (DEFAULT_DIRECTORY);
> +
> +  if (grub_devices == NULL || grub_devices[0] == NULL)
> +    {
> +      grub_util_warn (_("cannot find a device for %s (is /dev
> mounted?)"), DEFAULT_DIRECTORY);
> +      goto cleanup;
> +    }
> +
> +  devname = xstrdup (grub_devices[0]);

Using grub_strdup would be better. We can check for NULL return in case 
of failure!

> +
> +  for (curdev = grub_devices; *curdev; curdev++, ndev++)
> +    grub_util_pull_device (*curdev);
> +
> +  grub_drives = xcalloc ((ndev + 1), sizeof (grub_drives[0]));

Same as above. grub_calloc...

> +
> +  for (curdev = grub_devices, curdrive = grub_drives; *curdev; 
> curdev++,
> +       curdrive++)
> +    {
> +      *curdrive = grub_util_get_grub_dev (*curdev);
> +      if (*curdrive == NULL)
> +	{
> +	  grub_util_warn (_("cannot find a GRUB drive for %s.  Check your
> device.map"),
> +			  *curdev);
> +	  goto cleanup;
> +	}
> +    }
> +  *curdrive = 0;
> +
> +  grub_dev = grub_device_open (grub_drives[0]);
> +  if (grub_dev == NULL)
> +    {
> +      grub_util_warn (_("cannot open device %s: %s"), grub_drives[0],
> grub_errmsg);
> +      grub_errno = GRUB_ERR_NONE;
> +      goto cleanup;
> +    }
> +
> +  grub_fs = grub_fs_probe (grub_dev);
> +  if (grub_fs == NULL)
> +    {
> +      grub_util_warn (_("cannot probe fs for %s: %s"),
> grub_drives[0], grub_errmsg);
> +      grub_errno = GRUB_ERR_NONE;
> +      goto cleanup;
> +    }
> +
> +  have_abstraction = is_abstraction (grub_dev);
> +  for (curdrive = grub_drives + 1; *curdrive && have_abstraction ==
> false; curdrive++)
> +    {
> +      grub_device_t dev = grub_device_open (*curdrive);
> +      if (dev == NULL)
> +	continue;
> +      have_abstraction = is_abstraction (dev);
> +      grub_device_close (dev);
> +    }
> +
> + cleanup:
> +  if (grub_devices != NULL)
> +    for (curdev = grub_devices; *curdev; curdev++)
> +      free (*curdev);
> +  free (grub_devices);
> +  free (grub_drives);

grub_free() instead of free().

> +  grub_device_close (grub_dev);
> +  grub_gcry_fini_all ();
> +  grub_fini_all ();
> +  grub_util_biosdisk_fini ();
> +
> +  if (grub_fs == NULL)
> +    {
> +      free (devname);

grub_free() instead of free().

> +      return NULL;
> +    }
> +
> +  for (p = spec; p->fs_name; p++)
> +    {
> +      if (strcmp (grub_fs->name, p->fs_name) == 0 && 
> !have_abstraction)

Same as mentioned above. grub_strcmp()...

> +	{
> +	  fs_envblk = xmalloc (sizeof (fs_envblk_t));

Same as above. grub_malloc()...

> +	  fs_envblk->spec = p;
> +	  fs_envblk->dev = devname;
> +	  return fs_envblk;
> +	}
> +    }
> +
> +  free (devname);

grub_free() instead of free(). And adding a new line here would be good!

> +  return NULL;
> +}
> +
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -284,6 +432,9 @@ main (int argc, char *argv[])
>        command  = argv[curindex++];
>      }
> 
> +  if (strcmp (filename, DEFAULT_ENVBLK_PATH) == 0)

Same as mentioned above. grub_strcmp()...

Thank you!

Regards,
Avnish Chouhan

> +    fs_envblk = probe_fs_envblk (fs_envblk_spec);
> +
>    if (strcmp (command, "create") == 0)
>      grub_util_create_envblk_file (filename);
>    else if (strcmp (command, "list") == 0)
> --
> 2.51.0

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel