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

[Oliver Steffen]

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.

>
> > 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?

>
> > +/* boot_loader_interface.c - implementation of the boot loader interface
> > + */
>
> Please use correct formatting for comments [2] and move this one behind
> the license below.
>
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *
> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation, either version 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
>
> Please take license from the grub-core/kern/efi/sb.c file and update
> year in it. You can also merge module description with the license like
> it is done in the grub-core/kern/efi/sb.c.
>
> > +#include <grub/charset.h>
> > +#include <grub/efi/api.h>
> > +#include <grub/efi/disk.h>
> > +#include <grub/efi/efi.h>
> > +#include <grub/err.h>
> > +#include <grub/extcmd.h>
> > +#include <grub/gpt_partition.h>
> > +#include <grub/misc.h>
> > +#include <grub/mm.h>
> > +#include <grub/partition.h>
> > +
> > +GRUB_MOD_LICENSE ("GPLv3+");
> > +
> > +#define MODNAME "boot_loader_interface"
> > +
> > +static const grub_efi_guid_t boot_loader_interface_vendor_guid =
> > +  { 0x4a67b082, 0x0a4c, 0x41cf,
> > +    {0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f} };
>
> Please define this GUID as constant in the include/grub/efi/api.h (you
> will find good examples how it should be done there). I expect it can
> be useful for others too.
>
> > +static char *
> > +machine_get_bootdevice (void)
> > +{
> > +  grub_efi_loaded_image_t *image;
> > +
> > +  image = grub_efi_get_loaded_image (grub_efi_image_handle);
> > +  if (!image)
>
> Please compare pointers explicitly with NULL, e.g. "if (image == NULL)",
> here and below...
>
> > +    return NULL;
> > +
> > +  return grub_efidisk_get_device_name (image->device_handle);
> > +}
> > +
> > +static grub_err_t
> > +get_part_uuid (grub_device_t dev, char **part_uuid)
> > +{
> > +  grub_err_t status = GRUB_ERR_NONE;
> > +  grub_disk_t disk;
> > +  struct grub_gpt_partentry entry;
> > +  grub_gpt_part_guid_t *guid;
> > +
> > +  if (!dev || !dev->disk || !dev->disk->partition)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid device"));
> > +
> > +  disk = grub_disk_open (dev->disk->name);
> > +  if (!disk)
> > +    {
> > +      status = grub_errno;
> > +      grub_dprintf (MODNAME, "Error opening disk\n");
> > +      return grub_errno;
>
> s/return grub_errno/return status/?
>
> > +    }
> > +
> > +  if (grub_strcmp (dev->disk->partition->partmap->name, "gpt") != 0)
> > +    {
> > +      status = grub_error (GRUB_ERR_BAD_PART_TABLE,
> > +                           N_("This is not a GPT partition table"));
>
> All errors in the GRUB start with lower case, i.e. s/This is/this is/.
>
> > +      goto finish;
>
> s/finish/fail/ and below please...
>
> > +    }
> > +
> > +  if (grub_disk_read (disk, dev->disk->partition->offset,
> > +                      dev->disk->partition->index, sizeof (entry), &entry))
>
> ..., , &entry) != GRUB_ERR_NONE)
>
> > +    {
> > +      status = grub_errno;
> > +      grub_dprintf (MODNAME, "%s: Read error\n", dev->disk->name);
> > +      goto finish;
> > +    }
> > +
> > +  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.

> > +  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.

>
> > +  value_16 = grub_calloc (len16 + 1, sizeof (value_16[0]));
> > +  if (!value_16)
> > +    return grub_errno;
> > +
> > +  len16
> > +      = grub_utf8_to_utf16 (value_16, len16, (grub_uint8_t *)value, len, 
> > NULL);
>
> Please do not break lines in that way. It is difficult to read.
>
> Here you can just do
>
>   len16 = grub_utf8_to_utf16 (value_16, len16, (grub_uint8_t *)value, len, 
> NULL);
>
> > +  value_16[len16] = 0;
> > +
> > +  status = grub_efi_set_variable_with_attributes (
> > +      name, guid, (void *)value_16, (len16 + 1) * sizeof (value_16[0]),
>
> s/(void *)value_16/(void *) value_16/
>
> Casts require a space... Please fix this kinds of problems everywhere.
>
> > +      GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS | 
> > GRUB_EFI_VARIABLE_RUNTIME_ACCESS);
> > +  if (status != GRUB_ERR_NONE)
>
> Yeah, I like this explicit comparison...
>
> > +    {
> > +      grub_dprintf (MODNAME, "Error setting EFI variable %s: %d\n", name,
> > +                    status);
> > +    }
>
> Please do not add braces if they are not needed...
>
> > +  grub_free (value_16);
> > +
> > +  return status;
> > +}
> > +
> > +static grub_err_t
> > +set_loader_info (void)
> > +{
> > +  grub_err_t status;
>
> You do not need the status variable here. Just return immediately status from 
> the
> set_efi_str_variable() call.
>
> > +  status = set_efi_str_variable (
> > +      "LoaderInfo", &boot_loader_interface_vendor_guid, PACKAGE_STRING);
>
>   return set_efi_str_variable ("LoaderInfo", 
> &boot_loader_interface_vendor_guid, PACKAGE_STRING);
>
> I am OK with lines (a bit) longer than 80 chars when it increases readability.
>
> > +  return status;
> > +}
> > +
> > +static grub_err_t
> > +set_loader_device_part_uuid (void)
> > +{
> > +  grub_err_t status = GRUB_ERR_NONE;
> > +  char *device_name = NULL;
> > +  grub_device_t device;
> > +  char *part_uuid = NULL;
> > +
> > +  device_name = machine_get_bootdevice ();
> > +  if (!device_name)
> > +    {
> > +      return grub_error (GRUB_ERR_BAD_DEVICE,
> > +                         N_("Unable to find boot device"));
> > +    }
> > +
> > +  device = grub_device_open (device_name);
> > +  if (!device)
> > +    {
> > +      status = grub_errno;
> > +      grub_dprintf (MODNAME, "Error opening device: %s", device_name);
> > +      goto err;
>
> s/err/fail/ and below please...
>
> > +    }
> > +
> > +  status = get_part_uuid (device, &part_uuid);
> > +
> > +  grub_device_close (device);
> > +
> > +  if (status == GRUB_ERR_NONE)
> > +    {
> > +      status = set_efi_str_variable ("LoaderDevicePartUUID",
> > +                                     &boot_loader_interface_vendor_guid,
> > +                                     part_uuid);
> > +    }
> > +
> > +err:
>
> Missing space before label.
>
> > +  grub_free (part_uuid);
> > +  grub_free (device_name);
> > +  return status;
> > +}
> > +
> > +static grub_err_t
> > +grub_cmd_boot_loader_interface (grub_extcmd_context_t ctxt __attribute__ 
> > ((unused)),
> > +                                int argc __attribute__ ((unused)),
> > +                                char **args __attribute__ ((unused)))
> > +{
> > +  grub_err_t status;
> > +
> > +  status = set_loader_info ();
> > +  if (status != GRUB_ERR_NONE)
> > +    return status;
> > +
> > +  status = set_loader_device_part_uuid ();
> > +  if (status != GRUB_ERR_NONE)
> > +    return status;
> > +
> > +  return GRUB_ERR_NONE;
> > +}
> > +
> > +static grub_extcmd_t cmd;
> > +
> > +GRUB_MOD_INIT (boot_loader_interface)
> > +{
> > +  grub_dprintf (MODNAME, "%s got here\n", __func__);
> > +  cmd = grub_register_extcmd (
> > +      "boot_loader_interface", grub_cmd_boot_loader_interface, 0, NULL,
> > +      N_("Set EFI variables according to Boot Loader Interface spec."), 
> > NULL);
>
> This kind of formatting does not parse well...
>
>   cmd = grub_register_extcmd ("boot_loader_interface", 
> grub_cmd_boot_loader_interface, 0, NULL,
>                               N_("Set EFI variables according to Boot Loader 
> Interface spec."), NULL);
>
> Please fix similar problem everywhere.
>
> > +}
> > +
> > +GRUB_MOD_FINI (boot_loader_interface) { grub_unregister_extcmd (cmd); }
>
> GRUB_MOD_FINI (boot_loader_interface)
> {
>   grub_unregister_extcmd (cmd);
> }
>
> Daniel
>
> [1] 
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/spec_ctrl.c;h=e80e2a5ed1a9b61bce564f83baf95492ff17198d;hb=HEAD
> [2] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments
>

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

-Oliver