On Tue, Sep 02, 2025 at 02:45:58PM +0800, Michael Chang via Grub-devel wrote:
> This patch adds the logic to locate and open an environment block that
> is stored in a reserved area on the device. It introduces the function
> fs_envblk_open together with helper routines to read the block pointed
> to by the env_block variable, and to create the block on disk when it
> does not yet exist. When a block is created, the code records its
> location inside the file based envblk by setting env_block in block list
> syntax of offset plus size in sectors.
>
> The env_block variable acts as a link from the file envblk to the raw
> disk region so that later runs of grub editenv can follow it and access
> the external block. The helper is exposed through a small ops table
> attached to fs_envblk so that later patches can call
> fs_envblk->ops->open without touching core code again. At this stage
> variables are still stored in the file envblk and no redirection has
> been applied.
>
> Signed-off-by: Michael Chang <[email protected]>
> Reviewed-by: Neal Gompa <[email protected]>
> ---
> util/grub-editenv.c | 127 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 127 insertions(+)
>
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index a1fa711cd..7bc872dc7 100644
> --- a/util/grub-editenv.c
> +++ b/util/grub-editenv.c
> @@ -132,8 +132,17 @@ struct fs_envblk_spec {
> { NULL, 0, 0 }
> };
>
> +static grub_envblk_t fs_envblk_open (grub_envblk_t envblk);
> +
> +struct fs_envblk_ops {
> + grub_envblk_t (*open) (grub_envblk_t);
> +} fs_envblk_ops = {
> + .open = fs_envblk_open
> +};
> +
> struct fs_envblk {
> struct fs_envblk_spec *spec;
> + struct fs_envblk_ops *ops;
> const char *dev;
> };
>
> @@ -142,6 +151,123 @@ typedef struct fs_envblk *fs_envblk_t;
>
> fs_envblk_t fs_envblk = NULL;
>
> +static int
Why not bool?
> +read_envblk_fs (const char *varname, const char *value, void *hook_data)
> +{
> + grub_envblk_t *p_envblk = (grub_envblk_t *)hook_data;
> +
> + if (p_envblk == NULL || fs_envblk == NULL)
> + return 1;
> +
> + if (strcmp (varname, "env_block") == 0)
Return if !=0 and you can drop one indention below...
> + {
> + int off, sz;
> + char *p;
> +
> + off = strtol (value, &p, 10);
> + if (*p == '+')
> + sz = strtol (p+1, &p, 10);
> + else
> + return 0;
> +
> + if (*p == '\0')
> + {
> + FILE *fp;
> + char *buf;
> +
> + off <<= GRUB_DISK_SECTOR_BITS;
> + sz <<= GRUB_DISK_SECTOR_BITS;
> +
> + fp = grub_util_fopen (fs_envblk->dev, "rb");
> + if (! fp)
> + grub_util_error (_("cannot open `%s': %s"), fs_envblk->dev,
> + strerror (errno));
> +
> +
Please drop this redundant line...
> + if (fseek (fp, off, SEEK_SET) < 0)
> + grub_util_error (_("cannot seek `%s': %s"), fs_envblk->dev,
> + strerror (errno));
> +
> + buf = xmalloc (sz);
> + if ((fread (buf, 1, sz, fp)) != sz)
> + grub_util_error (_("cannot read `%s': %s"), fs_envblk->dev,
> + strerror (errno));
You blindly trust off and sz values. I think you should check limits here.
Or somewhere else...
> +
> + fclose (fp);
> +
> + *p_envblk = grub_envblk_open (buf, sz);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void
> +create_envblk_fs (void)
> +{
> + FILE *fp;
> + char *buf;
> + const char *device;
> + int offset, size;
> +
> + if (fs_envblk == NULL)
> + return;
> +
> + device = fs_envblk->dev;
> + offset = fs_envblk->spec->offset;
> + size = fs_envblk->spec->size;
> +
> + fp = grub_util_fopen (device, "r+b");
> + if (! fp)
fp == NULL please...
> + grub_util_error (_("cannot open `%s': %s"), device, strerror (errno));
> +
> + buf = xmalloc (size);
> + memcpy (buf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1);
> + memset (buf + sizeof (GRUB_ENVBLK_SIGNATURE) - 1, '#', size - sizeof
> (GRUB_ENVBLK_SIGNATURE) + 1);
GRUB_ENVBLK_SIGNATURE definition is missing in this patch...
> + if (fseek (fp, offset, SEEK_SET) < 0)
> + grub_util_error (_("cannot seek `%s': %s"), device, strerror (errno));
> +
> + if (fwrite (buf, 1, size, fp) != size)
> + grub_util_error (_("cannot write to `%s': %s"), device, strerror
> (errno));
> +
> + grub_util_file_sync (fp);
> + free (buf);
> + fclose (fp);
> +}
> +
> +static grub_envblk_t
> +fs_envblk_open (grub_envblk_t envblk)
> +{
> + grub_envblk_t envblk_fs = NULL;
> + char *val;
> + int offset, size;
> +
> + if (envblk == NULL)
> + return NULL;
> +
> + offset = fs_envblk->spec->offset;
> + size = fs_envblk->spec->size;
I think offset should be defined as off_t, size as size_t,
fs_envblk->spec->offset, and fs_envblk->spec->size accordingly.
Daniel
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
