Re: [PATCH v1 2/2] Add a module for the Boot Loader Interface

[Daniel Kiper]

On Mon, Feb 20, 2023 at 08:15:35AM -0800, Oliver Steffen wrote:
> Thank you for the comments, Daniel.
>
> Quoting Daniel Kiper (2023-02-15 19:27:03)
> > On Mon, Jan 16, 2023 at 12:40:53PM +0100, Oliver Steffen wrote:
> > > Add a new module named boot_loader_interface, which provides a command
> >
> > I would prefer something shorter than boot_loader_interface. bli?
>
> Then bli it is.

Cool! Thanks!

> > > with the same name. It implements a small but quite useful part of the
> > > Boot Loader Interface [0].  This interface uses EFI variables for
> > > communication between the boot loader and the operating system.
> > >
> > > This module sets two EFI variables under the vendor GUID
> > > 4a67b082-0a4c-41cf-b6c7-440b29bb8c4f:
> > >
> > > - LoaderInfo: contains GRUB + <version number>.
> > >   This allows the running operating system to identify the boot loader
> > >   used during boot.
> > >
> > > - LoaderDevicePartUUID: contains the partition UUID of the
> > >   EFI System Partition (ESP).  This is used by
> > >   systemd-gpt-auto-generator [1] to find the root partitions (and others
> > >   too), via partition type IDs [2].
> > >
> > > This module is only available on EFI platforms.
> > >
> > > [0] https://systemd.io/BOOT_LOADER_INTERFACE/
> > > [1] 
> > > https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generator.html
> > > [2] 
> > > https://uapi-group.org/specifications/specs/discoverable_partitions_specification/
> > >
> > > Signed-off-by: Oliver Steffen <osteffen@redhat.com>
> > > ---
> > >  grub-core/Makefile.core.def                |   6 +
> > >  grub-core/commands/boot_loader_interface.c | 217 +++++++++++++++++++++
> > >  2 files changed, 223 insertions(+)
> > >  create mode 100644 grub-core/commands/boot_loader_interface.c
> > >
> > > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > > index ba967aac8..23455fb71 100644
> > > --- a/grub-core/Makefile.core.def
> > > +++ b/grub-core/Makefile.core.def
> > > @@ -2547,3 +2547,9 @@ module = {
> > >    common = commands/i386/wrmsr.c;
> > >    enable = x86;
> > >  };
> > > +
> > > +module = {
> > > +  name = boot_loader_interface;
> >
> > s/boot_loader_interface/bli/?
> >
> > > +  efi = commands/boot_loader_interface.c;
> >
> > Ditto and below if needed...
> >
> > > +  enable = efi;
> > > +};
> > > diff --git a/grub-core/commands/boot_loader_interface.c 
> > > b/grub-core/commands/boot_loader_interface.c
> > > new file mode 100644
> > > index 000000000..ccd7fa3d9
> > > --- /dev/null
> > > +++ b/grub-core/commands/boot_loader_interface.c
> > > @@ -0,0 +1,217 @@
> > > +/*-*- Mode: C; c-basic-offset: 2; indent-tabs-mode: t -*-*/
> >
> > Could you move this to the end of the file? Good example you can find in
> > the Xen project [1].
>
> I personally do not care about this, it was just part of the file I used
> as a template. Is this wanted, or can we just drop it?

If you do not need it please drop it.

[...]

> > > +  guid = &entry.guid;
> > > +  *part_uuid = grub_xasprintf (
> > > +      "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> > > +      grub_le_to_cpu32 (guid->data1), grub_le_to_cpu16 (guid->data2),
> > > +      grub_le_to_cpu16 (guid->data3), guid->data4[0], guid->data4[1],
> > > +      guid->data4[2], guid->data4[3], guid->data4[4], guid->data4[5],
> > > +      guid->data4[6], guid->data4[7]);
> >
> > I think this is generic thing and can be converted into a function.
> > Similar code is in the grub-core/commands/probe.c and util/grub-probe.c.
> > It seems to me it is at least easy to make one function for the GRUB
> > core code. Please do that. Of course in separate patch.
>
> There are multiple representations of a GUID:
>  - grub_gpt_part_guid (gpt_partition.h)
>  - grub_efi_guid (efi/api.h)
>  - grub_efi_packed_guid (efi/api.h)
>
> I would add a function for the first kind to gpt_partition.h and
> partmap/gpt.c that prints into a string buffer (length checked).
>
> It would be nice to add a format specifier for GUIDs to the printf
> implementation,
> but that only makes much sense (I think) if the GUID repesentations
> could be unified into one first, in a central place.

Could you do the unification and introduce format specifier for GUIDs?

> > > +  if (!*part_uuid)
> > > +    {
> > > +      status = grub_errno;
> > > +    }
> >
> > Please drop redundant braces.
> >
> > > +finish:
> >
> > Labels should be prefixed with a space...
> >
> > > +  grub_disk_close (disk);
> > > +
> > > +  return status;
> > > +}
> > > +
> > > +static grub_err_t
> > > +set_efi_str_variable (const char *name, const grub_efi_guid_t *guid,
> > > +                      const char *value)
> > > +{
> > > +  grub_size_t len;
> > > +  grub_size_t len16;
> >
> > grub_size_t len, len16;
> >
> > > +  grub_efi_char16_t *value_16;
> > > +  grub_err_t status;
> > > +
> > > +  len = grub_strlen (value);
> > > +  len16 = len * GRUB_MAX_UTF16_PER_UTF8;
> >
> > What will happen when len == 0?
> >
> > Is there any chance for overflow here? If yes please use overflow aware
> > operators from include/grub/safemath.h here.
>
> Fixing this.  Same problem also in grub_efi_set_variable_with_attributes.
> This is were I found this code.  Fill fix there too.

Cool! Thanks a lot!

> Thanks for the review.  I will send out v2 shortly.

Great!

Thanks,

Daniel