On Fri, Sep 05, 2025 at 10:08:41PM +0200, Daniel Kiper wrote:
> 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?
OK. Will change it.
>
> > +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...
Yes. Will change it.
>
> > + {
> > + 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...
OK.
>
> > + 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...
I consider it trusted because the off and sz values are taken from the
env_block variable, which is read-only. This variable is only set at the
time the /boot/grub/grubenv file is created, using predefined values
from the fs_envblk_spec table. The related implementation can be found
in a later patch:
"util/grub-editenv: wire set_variables to optional fs_envblk"
In addition, grub_envblk_open() validates the data read from (off, sz)
and discards any invalid results.
>
> > +
> > + 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...
OK.
>
> > + 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...
I think the definition has been present and used in the source before
this patch series.
>
> > + 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.
OK. I will do it.
Thanks,
Michael
>
> Daniel
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
