[PATCH v2 0/7] Add basic Boot Loader Interface support

[PATCH v2 0/7] Add basic Boot Loader Interface support

[Oliver Steffen]

This is a step towards supporting unified kernel images (UKI) in Grub.

Add a new module named bli, which provides a command 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 is especially useful for
  UKIs, where the kernel command line is fixed and usually does not
  contain any information about the root partition.

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/

v2:
- Addressed comments from Daniel
- Added a print function for gpt guids
- Added integer overflow check in UTF16 conversion
- Added config drop-in file that loads the module on EFI

v1:
https://mail.gnu.org/archive/html/grub-devel/2023-01/msg00104.html

Oliver Steffen (7):
  efi: add grub_efi_set_variable_with_attributes
  efi: check for integer overflow in string conversion
  Add a module for the Boot Loader Interface
  util/grub.d: activate bli module on EFI
  partmap/gpt: add print function for guids
  commands/probe: use grub_gpt_part_guid_snprint
  commands/bli: use grub_gpt_part_guid_snprint

 grub-core/Makefile.core.def             |   6 +
 grub-core/commands/bli.c                | 209 ++++++++++++++++++++++++
 grub-core/commands/probe.c              |  11 +-
 grub-core/kern/efi/efi.c                |  25 ++-
 grub-core/partmap/gpt.c                 |  13 ++
 include/grub/efi/api.h                  |   5 +
 include/grub/efi/efi.h                  |   6 +
 include/grub/gpt_partition.h            |   6 +
 util/grub.d/25_boot_loader_interface.in |  34 ++++
 9 files changed, 300 insertions(+), 15 deletions(-)
 create mode 100644 grub-core/commands/bli.c
 create mode 100644 util/grub.d/25_boot_loader_interface.in

-- 
2.39.2

[PATCH v2 1/7] efi: add grub_efi_set_variable_with_attributes

[Oliver Steffen]

Add a function to the EFI module that allows setting EFI variables
with specific attributes.

This is useful for marking variables as volatile, for example.

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
---
 grub-core/kern/efi/efi.c | 19 +++++++++++++------
 include/grub/efi/efi.h   |  6 ++++++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index cf49d6357..03abf5531 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -201,8 +201,8 @@ grub_efi_set_virtual_address_map (grub_efi_uintn_t memory_map_size,
 }
 
 grub_err_t
-grub_efi_set_variable(const char *var, const grub_efi_guid_t *guid,
-		      void *data, grub_size_t datasize)
+grub_efi_set_variable_with_attributes (const char *var, const grub_efi_guid_t *guid,
+		      void *data, grub_size_t datasize, grub_efi_uint32_t attributes)
 {
   grub_efi_status_t status;
   grub_efi_runtime_services_t *r;
@@ -219,10 +219,7 @@ grub_efi_set_variable(const char *var, const grub_efi_guid_t *guid,
 
   r = grub_efi_system_table->runtime_services;
 
-  status = efi_call_5 (r->set_variable, var16, guid,
-		       (GRUB_EFI_VARIABLE_NON_VOLATILE
-			| GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS
-			| GRUB_EFI_VARIABLE_RUNTIME_ACCESS),
+  status = efi_call_5 (r->set_variable, var16, guid, attributes,
 		       datasize, data);
   grub_free (var16);
   if (status == GRUB_EFI_SUCCESS)
@@ -231,6 +228,16 @@ grub_efi_set_variable(const char *var, const grub_efi_guid_t *guid,
   return grub_error (GRUB_ERR_IO, "could not set EFI variable `%s'", var);
 }
 
+grub_err_t
+grub_efi_set_variable (const char *var, const grub_efi_guid_t *guid,
+		      void *data, grub_size_t datasize)
+{
+  return grub_efi_set_variable_with_attributes (var, guid, data, datasize, 
+			GRUB_EFI_VARIABLE_NON_VOLATILE
+			| GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS
+			| GRUB_EFI_VARIABLE_RUNTIME_ACCESS);
+}
+
 grub_efi_status_t
 grub_efi_get_variable_with_attributes (const char *var,
 				       const grub_efi_guid_t *guid,
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index e61272de5..8e9a905a4 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -86,6 +86,12 @@ grub_efi_status_t EXPORT_FUNC (grub_efi_get_variable) (const char *variable,
 						       grub_size_t *datasize_out,
 						       void **data_out);
 grub_err_t
+EXPORT_FUNC (grub_efi_set_variable_with_attributes) (const char *var,
+				     const grub_efi_guid_t *guid,
+				     void *data,
+				     grub_size_t datasize,
+				     grub_efi_uint32_t attributes);
+grub_err_t
 EXPORT_FUNC (grub_efi_set_variable) (const char *var,
 				     const grub_efi_guid_t *guid,
 				     void *data,
-- 
2.39.2

[PATCH v2 2/7] efi: check for integer overflow in string conversion

[Oliver Steffen]

Check for integer overflow when converting the name of the
EFI variable to UTF16 in grub_efi_set_variable_with_attributes().

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
---
 grub-core/kern/efi/efi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 03abf5531..a23c80a21 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -25,6 +25,7 @@
 #include <grub/efi/pe32.h>
 #include <grub/time.h>
 #include <grub/term.h>
+#include <grub/types.h>
 #include <grub/kernel.h>
 #include <grub/mm.h>
 #include <grub/loader.h>
@@ -210,6 +211,11 @@ grub_efi_set_variable_with_attributes (const char *var, const grub_efi_guid_t *g
   grub_size_t len, len16;
 
   len = grub_strlen (var);
+
+  /* Check for integer overflow */
+  if (len > GRUB_SIZE_MAX / GRUB_MAX_UTF16_PER_UTF8 - 1)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("variable name too long"));
+
   len16 = len * GRUB_MAX_UTF16_PER_UTF8;
   var16 = grub_calloc (len16 + 1, sizeof (var16[0]));
   if (!var16)
-- 
2.39.2

[PATCH v2 3/7] Add a module for the Boot Loader Interface

[Oliver Steffen]

Add a new module named boot_loader_interface, which provides a command
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/bli.c    | 213 ++++++++++++++++++++++++++++++++++++
 include/grub/efi/api.h      |   5 +
 3 files changed, 224 insertions(+)
 create mode 100644 grub-core/commands/bli.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 71093a100..cdfa2d101 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2548,3 +2548,9 @@ module = {
   common = commands/i386/wrmsr.c;
   enable = x86;
 };
+
+module = {
+  name = bli;
+  efi = commands/bli.c;
+  enable = efi;
+};
diff --git a/grub-core/commands/bli.c b/grub-core/commands/bli.c
new file mode 100644
index 000000000..10993222d
--- /dev/null
+++ b/grub-core/commands/bli.c
@@ -0,0 +1,213 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2023  Free Software Foundation, Inc.
+ *
+ *  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/>.
+ *
+ *  Implementation of the Boot Loader Interface.
+ */
+
+#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>
+#include <grub/types.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+#define MODNAME "bli"
+
+static const grub_efi_guid_t bli_vendor_guid = GRUB_EFI_VENDOR_BOOT_LOADER_INTERFACE_GUID;
+
+static char *
+machine_get_bootdevice (void)
+{
+  grub_efi_loaded_image_t *image;
+
+  image = grub_efi_get_loaded_image (grub_efi_image_handle);
+  if (image == NULL)
+    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 == NULL || dev->disk == NULL || dev->disk->partition == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid device"));
+
+  disk = grub_disk_open (dev->disk->name);
+  if (disk == NULL)
+    {
+      status = grub_errno;
+      grub_dprintf (MODNAME, "Error opening disk\n");
+      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"));
+      goto fail;
+    }
+
+  if (grub_disk_read (disk, dev->disk->partition->offset,
+		      dev->disk->partition->index, sizeof (entry), &entry) != GRUB_ERR_NONE)
+    {
+      status = grub_errno;
+      grub_dprintf (MODNAME, "%s: Read error\n", dev->disk->name);
+      goto fail;
+    }
+
+  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]);
+  if (*part_uuid == NULL)
+    status = grub_errno;
+
+ fail:
+  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, len16;
+  grub_efi_char16_t *value_16;
+  grub_err_t status;
+
+  len = grub_strlen (value);
+
+  /* Check for integer overflow */
+  if (len > GRUB_SIZE_MAX / GRUB_MAX_UTF16_PER_UTF8 - 1)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("data too large"));
+
+  len16 = len * GRUB_MAX_UTF16_PER_UTF8;
+
+  value_16 = grub_calloc (len16 + 1, sizeof (value_16[0]));
+  if (value_16 == NULL)
+    return grub_errno;
+
+  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]),
+			GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS
+			| GRUB_EFI_VARIABLE_RUNTIME_ACCESS);
+  if (status != GRUB_ERR_NONE)
+    grub_dprintf (MODNAME, "Error setting EFI variable %s: %d\n", name, status);
+
+  grub_free (value_16);
+
+  return status;
+}
+
+static grub_err_t
+set_loader_info (void)
+{
+  return set_efi_str_variable ("LoaderInfo", &bli_vendor_guid, PACKAGE_STRING);
+}
+
+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 == NULL)
+    return grub_error (GRUB_ERR_BAD_DEVICE, N_("unable to find boot device"));
+
+  device = grub_device_open (device_name);
+  if (device == NULL)
+    {
+      status = grub_errno;
+      grub_dprintf (MODNAME, "Error opening device: %s", device_name);
+      goto fail;
+    }
+
+  status = get_part_uuid (device, &part_uuid);
+
+  grub_device_close (device);
+
+  if (status == GRUB_ERR_NONE)
+    status = set_efi_str_variable ("LoaderDevicePartUUID",
+				   &bli_vendor_guid,
+				   part_uuid);
+
+ fail:
+  grub_free (part_uuid);
+  grub_free (device_name);
+  return status;
+}
+
+static grub_err_t
+grub_cmd_bli (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 (bli)
+{
+  grub_dprintf (MODNAME, "%s got here\n", __func__);
+  cmd = grub_register_extcmd (
+	  "bli",
+	  grub_cmd_bli,
+	  0,
+	  NULL,
+	  N_("Set EFI variables according to Boot Loader Interface spec."),
+	  NULL);
+}
+
+GRUB_MOD_FINI (bli)
+{
+  grub_unregister_extcmd (cmd);
+}
+
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index ad2680341..2835b4098 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -374,6 +374,11 @@
     { 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 } \
   }
 
+#define GRUB_EFI_VENDOR_BOOT_LOADER_INTERFACE_GUID \
+  { 0x4a67b082, 0x0a4c, 0x41cf, \
+    {0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f } \
+  }
+
 struct grub_efi_sal_system_table
 {
   grub_uint32_t signature;
-- 
2.39.2

[PATCH v2 4/7] util/grub.d: activate bli module on EFI

[Oliver Steffen]

Add a new configuration drop-in file that loads the
boot-loader-interface (bli) module and runs the command in case we are
booting on  the EFI platform.

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
---
 util/grub.d/25_boot_loader_interface.in | 34 +++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 util/grub.d/25_boot_loader_interface.in

diff --git a/util/grub.d/25_boot_loader_interface.in b/util/grub.d/25_boot_loader_interface.in
new file mode 100644
index 000000000..8285d7627
--- /dev/null
+++ b/util/grub.d/25_boot_loader_interface.in
@@ -0,0 +1,34 @@
+#!/usr/bin/sh
+set -e
+
+# grub-mkconfig helper script.
+# Copyright (C) 2020  Free Software Foundation, Inc.
+#
+# 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/>.
+
+prefix="/usr"
+exec_prefix="/usr"
+datarootdir="/usr/share"
+
+export TEXTDOMAIN=grub
+export TEXTDOMAINDIR="${datarootdir}/locale"
+
+. "$pkgdatadir/grub-mkconfig_lib"
+
+cat << EOF
+if [ "\$grub_platform" = "efi" ]; then
+  insmod boot-loader-interface
+  boot-loader-interface
+fi
+EOF
-- 
2.39.2

[PATCH v2 5/7] partmap/gpt: add print function for guids

[Oliver Steffen]

Code to print gpt patition guids has been duplicated in multiple places.
Add a common function for that.

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
---
 grub-core/partmap/gpt.c      | 13 +++++++++++++
 include/grub/gpt_partition.h |  6 ++++++
 2 files changed, 19 insertions(+)

diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
index 075cc96f1..e0da7e885 100644
--- a/grub-core/partmap/gpt.c
+++ b/grub-core/partmap/gpt.c
@@ -227,6 +227,19 @@ static struct grub_partition_map grub_gpt_partition_map =
 #endif
   };
 
+int
+grub_gpt_part_guid_snprint (char *str, grub_size_t n, const grub_gpt_part_guid_t *guid)
+{
+  return grub_snprintf (str, n,
+			 GRUB_PRIxGPT_GUID,
+			 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]);
+}
+
 GRUB_MOD_INIT(part_gpt)
 {
   grub_partition_map_register (&grub_gpt_partition_map);
diff --git a/include/grub/gpt_partition.h b/include/grub/gpt_partition.h
index 7a93f4329..9b1660322 100644
--- a/include/grub/gpt_partition.h
+++ b/include/grub/gpt_partition.h
@@ -50,6 +50,9 @@ typedef struct grub_gpt_part_guid grub_gpt_part_guid_t;
 	{ 0x85, 0xD2, 0xE1, 0xE9, 0x04, 0x34, 0xCF, 0xB3 }	\
   }
 
+#define GRUB_PRIxGPT_GUID "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x"
+#define GRUB_GPT_GUID_STR_LEN 36
+
 struct grub_gpt_header
 {
   grub_uint8_t magic[8];
@@ -83,5 +86,8 @@ grub_gpt_partition_map_iterate (grub_disk_t disk,
 				grub_partition_iterate_hook_t hook,
 				void *hook_data);
 
+int
+grub_gpt_part_guid_snprint (char *str, grub_size_t n,
+			    const grub_gpt_part_guid_t *guid);
 
 #endif /* ! GRUB_GPT_PARTITION_HEADER */
-- 
2.39.2

[PATCH v2 6/7] commands/probe: use grub_gpt_part_guid_snprint

[Oliver Steffen]

Use the new function for printing the partition guid.

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
---
 grub-core/commands/probe.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/grub-core/commands/probe.c b/grub-core/commands/probe.c
index 9a80ea54f..3fe50d11e 100644
--- a/grub-core/commands/probe.c
+++ b/grub-core/commands/probe.c
@@ -104,7 +104,7 @@ grub_cmd_probe (grub_extcmd_context_t ctxt, int argc, char **args)
   if (state[6].set)
     {
       /* AAAABBBB-CCCC-DDDD-EEEE-FFFFFFFFFFFF + null terminator */
-      char val[37] = "none";
+      char val[GRUB_GPT_GUID_STR_LEN + 1] = "none";
       if (dev->disk && dev->disk->partition)
 	{
 	  struct grub_partition *p = dev->disk->partition;
@@ -130,14 +130,7 @@ grub_cmd_probe (grub_extcmd_context_t ctxt, int argc, char **args)
 		  return grub_errno;
 		}
 	      guid = &entry.guid;
-	      grub_snprintf (val, sizeof(val),
-			     "%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]);
+        grub_gpt_part_guid_snprint (val, sizeof(val), guid);
 	    }
 	  else if (grub_strcmp(dev->disk->partition->partmap->name, "msdos") == 0)
 	    {
-- 
2.39.2

[PATCH v2 7/7] commands/bli: use grub_gpt_part_guid_snprint

[Oliver Steffen]

Use the new function for printing the partition guid.

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
---
 grub-core/commands/bli.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/grub-core/commands/bli.c b/grub-core/commands/bli.c
index 10993222d..160cbdb34 100644
--- a/grub-core/commands/bli.c
+++ b/grub-core/commands/bli.c
@@ -83,14 +83,10 @@ get_part_uuid (grub_device_t dev, char **part_uuid)
     }
 
   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]);
+  *part_uuid = grub_calloc (GRUB_GPT_GUID_STR_LEN + 1, sizeof (char));
   if (*part_uuid == NULL)
     status = grub_errno;
+  grub_gpt_part_guid_snprint (*part_uuid, GRUB_GPT_GUID_STR_LEN + 1, guid);
 
  fail:
   grub_disk_close (disk);
-- 
2.39.2

Re: [PATCH v2 4/7] util/grub.d: activate bli module on EFI

[Javier Martinez Canillas]

Oliver Steffen <osteffen@redhat.com> writes:

Hello Oliver,

> Add a new configuration drop-in file that loads the
> boot-loader-interface (bli) module and runs the command in case we are
> booting on  the EFI platform.
>
> Signed-off-by: Oliver Steffen <osteffen@redhat.com>
> ---

[...]

> +
> +cat << EOF
> +if [ "\$grub_platform" = "efi" ]; then
> +  insmod boot-loader-interface
> +  boot-loader-interface
> +fi

Just a drop by comment, you renamed in v2 the module and command to
"bli", so this snippet won't work anymore. I also noticed that you
missed to update the module and command name in some commit messages
as well.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 4/7] util/grub.d: activate bli module on EFI

[Oliver Steffen]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

On Sat, Feb 25, 2023 at 8:41 PM Javier Martinez Canillas <javierm@redhat.com>
wrote:

> Oliver Steffen <osteffen@redhat.com> writes:
>
> Hello Oliver,
>
> > Add a new configuration drop-in file that loads the
> > boot-loader-interface (bli) module and runs the command in case we are
> > booting on  the EFI platform.
> >
> > Signed-off-by: Oliver Steffen <osteffen@redhat.com>
> > ---
>
> [...]
>
> > +
> > +cat << EOF
> > +if [ "\$grub_platform" = "efi" ]; then
> > +  insmod boot-loader-interface
> > +  boot-loader-interface
> > +fi
>
> Just a drop by comment, you renamed in v2 the module and command to
> "bli", so this snippet won't work anymore. I also noticed that you
> missed to update the module and command name in some commit messages
> as well.
>

Yes! Thanks! I'll fix that in the next round :-)

-Oliver



> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>
>

[-- Attachment #2: Type: text/html, Size: 1883 bytes --]

Re: [PATCH v2 1/7] efi: add grub_efi_set_variable_with_attributes

[Daniel Kiper]

On Mon, Feb 20, 2023 at 07:56:24PM +0100, Oliver Steffen wrote:
> Add a function to the EFI module that allows setting EFI variables
> with specific attributes.
>
> This is useful for marking variables as volatile, for example.
>
> Signed-off-by: Oliver Steffen <osteffen@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 2/7] efi: check for integer overflow in string conversion

[Daniel Kiper]

On Mon, Feb 20, 2023 at 07:56:25PM +0100, Oliver Steffen wrote:
> Check for integer overflow when converting the name of the
> EFI variable to UTF16 in grub_efi_set_variable_with_attributes().
>
> Signed-off-by: Oliver Steffen <osteffen@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 3/7] Add a module for the Boot Loader Interface

[Daniel Kiper]

On Mon, Feb 20, 2023 at 07:56:26PM +0100, Oliver Steffen wrote:
> Add a new module named boot_loader_interface, which provides a command
> 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/bli.c    | 213 ++++++++++++++++++++++++++++++++++++
>  include/grub/efi/api.h      |   5 +
>  3 files changed, 224 insertions(+)
>  create mode 100644 grub-core/commands/bli.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 71093a100..cdfa2d101 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -2548,3 +2548,9 @@ module = {
>    common = commands/i386/wrmsr.c;
>    enable = x86;
>  };
> +
> +module = {
> +  name = bli;
> +  efi = commands/bli.c;
> +  enable = efi;
> +};
> diff --git a/grub-core/commands/bli.c b/grub-core/commands/bli.c
> new file mode 100644
> index 000000000..10993222d
> --- /dev/null
> +++ b/grub-core/commands/bli.c
> @@ -0,0 +1,213 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2023  Free Software Foundation, Inc.
> + *
> + *  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/>.
> + *
> + *  Implementation of the Boot Loader Interface.
> + */
> +
> +#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>
> +#include <grub/types.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +#define MODNAME "bli"
> +
> +static const grub_efi_guid_t bli_vendor_guid = GRUB_EFI_VENDOR_BOOT_LOADER_INTERFACE_GUID;
> +
> +static char *
> +machine_get_bootdevice (void)
> +{
> +  grub_efi_loaded_image_t *image;
> +
> +  image = grub_efi_get_loaded_image (grub_efi_image_handle);
> +  if (image == NULL)
> +    return NULL;
> +
> +  return grub_efidisk_get_device_name (image->device_handle);
> +}

Do we really need this in a function? I am not convinced.

> +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 == NULL || dev->disk == NULL || dev->disk->partition == NULL)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid device"));
> +
> +  disk = grub_disk_open (dev->disk->name);
> +  if (disk == NULL)
> +    {
> +      status = grub_errno;
> +      grub_dprintf (MODNAME, "Error opening disk\n");

I would print a name of disk here if it is not NULL or empty. Good
example how it should be done, more or less, you can find in the commit
5464e31a4 (disk/plainmount: Support plain encryption mode).

> +      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"));

Probably ditto and probably below too... :-)

> +      goto fail;
> +    }
> +
> +  if (grub_disk_read (disk, dev->disk->partition->offset,
> +		      dev->disk->partition->index, sizeof (entry), &entry) != GRUB_ERR_NONE)
> +    {
> +      status = grub_errno;
> +      grub_dprintf (MODNAME, "%s: Read error\n", dev->disk->name);
> +      goto fail;
> +    }
> +
> +  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]);
> +  if (*part_uuid == NULL)
> +    status = grub_errno;
> +
> + fail:
> +  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, len16;
> +  grub_efi_char16_t *value_16;
> +  grub_err_t status;
> +
> +  len = grub_strlen (value);
> +

Taking into account earlier patch I think everything starting from here...

> +  /* Check for integer overflow */
> +  if (len > GRUB_SIZE_MAX / GRUB_MAX_UTF16_PER_UTF8 - 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("data too large"));
> +
> +  len16 = len * GRUB_MAX_UTF16_PER_UTF8;
> +
> +  value_16 = grub_calloc (len16 + 1, sizeof (value_16[0]));
> +  if (value_16 == NULL)
> +    return grub_errno;
> +
> +  len16 = grub_utf8_to_utf16 (value_16, len16, (grub_uint8_t *) value, len, NULL);
> +  value_16[len16] = 0;

"value_16[len16] = 0;" seems redundant here. The grub_calloc() returns
pointer to zeroed memory region.

... until here is repeated at least twice in the GRUB code. Could you
put it in a function and replace similar code in the GRUB everywhere?

Hmmm... Probably similar is true for UTF-8 -> UTF-16 conversion...
I would be more than grateful if you fix it too.

> +  status = grub_efi_set_variable_with_attributes (name, guid,
> +			(void *) value_16, (len16 + 1) * sizeof (value_16[0]),
> +			GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS
> +			| GRUB_EFI_VARIABLE_RUNTIME_ACCESS);
> +  if (status != GRUB_ERR_NONE)
> +    grub_dprintf (MODNAME, "Error setting EFI variable %s: %d\n", name, status);
> +
> +  grub_free (value_16);
> +
> +  return status;
> +}
> +
> +static grub_err_t
> +set_loader_info (void)
> +{
> +  return set_efi_str_variable ("LoaderInfo", &bli_vendor_guid, PACKAGE_STRING);
> +}

I think there is no point to have this function. Please drop it and use
set_efi_str_variable() directly below.

> +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 == NULL)
> +    return grub_error (GRUB_ERR_BAD_DEVICE, N_("unable to find boot device"));
> +
> +  device = grub_device_open (device_name);
> +  if (device == NULL)
> +    {
> +      status = grub_errno;
> +      grub_dprintf (MODNAME, "Error opening device: %s", device_name);
> +      goto fail;
> +    }
> +
> +  status = get_part_uuid (device, &part_uuid);
> +
> +  grub_device_close (device);
> +
> +  if (status == GRUB_ERR_NONE)
> +    status = set_efi_str_variable ("LoaderDevicePartUUID",
> +				   &bli_vendor_guid,
> +				   part_uuid);
> +
> + fail:
> +  grub_free (part_uuid);
> +  grub_free (device_name);
> +  return status;
> +}
> +
> +static grub_err_t
> +grub_cmd_bli (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 (bli)
> +{
> +  grub_dprintf (MODNAME, "%s got here\n", __func__);
> +  cmd = grub_register_extcmd (
> +	  "bli",
> +	  grub_cmd_bli,
> +	  0,
> +	  NULL,
> +	  N_("Set EFI variables according to Boot Loader Interface spec."),
> +	  NULL);

This does not parse. Please use form from below...

cmd = grub_register_extcmd ("bli", grub_cmd_bli, 0, NULL,
                            N_("Set EFI variables according to Boot Loader Interface spec."), NULL);

Daniel

Re: [PATCH v2 5/7] partmap/gpt: add print function for guids

[Daniel Kiper]

On Mon, Feb 20, 2023 at 07:56:28PM +0100, Oliver Steffen wrote:
> Code to print gpt patition guids has been duplicated in multiple places.
> Add a common function for that.
>
> Signed-off-by: Oliver Steffen <osteffen@redhat.com>
> ---
>  grub-core/partmap/gpt.c      | 13 +++++++++++++
>  include/grub/gpt_partition.h |  6 ++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> index 075cc96f1..e0da7e885 100644
> --- a/grub-core/partmap/gpt.c
> +++ b/grub-core/partmap/gpt.c
> @@ -227,6 +227,19 @@ static struct grub_partition_map grub_gpt_partition_map =
>  #endif
>    };
>
> +int
> +grub_gpt_part_guid_snprint (char *str, grub_size_t n, const grub_gpt_part_guid_t *guid)

The GUIDs are used not only for disks. They are generic thing and this
function should be put in the common __non-EFI__ GRUB code.

> +{
> +  return grub_snprintf (str, n,
> +			 GRUB_PRIxGPT_GUID,
> +			 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]);
> +}
> +
>  GRUB_MOD_INIT(part_gpt)
>  {
>    grub_partition_map_register (&grub_gpt_partition_map);
> diff --git a/include/grub/gpt_partition.h b/include/grub/gpt_partition.h
> index 7a93f4329..9b1660322 100644
> --- a/include/grub/gpt_partition.h
> +++ b/include/grub/gpt_partition.h
> @@ -50,6 +50,9 @@ typedef struct grub_gpt_part_guid grub_gpt_part_guid_t;
>  	{ 0x85, 0xD2, 0xE1, 0xE9, 0x04, 0x34, 0xCF, 0xB3 }	\
>    }
>
> +#define GRUB_PRIxGPT_GUID "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x"

s/GRUB_PRIxGPT_GUID/PRIxGRUB_GUID/

> +#define GRUB_GPT_GUID_STR_LEN 36

#define GRUB_GUID_STRLEN (sizeof("AAAABBBB-CCCC-DDDD-EEEE-FFFFFFFFFFFF") - 1)

The result is the same but everybody knows what you are aiming for.
(stolen from grub-core/commands/probe.c; you could replace
  char val[37] = "none";
with
  char val[GRUB_GUID_STRLEN + 1] = "none";
there).

Daniel

Re: [PATCH v2 6/7] commands/probe: use grub_gpt_part_guid_snprint

[Daniel Kiper]

On Mon, Feb 20, 2023 at 07:56:29PM +0100, Oliver Steffen wrote:
> Use the new function for printing the partition guid.
>
> Signed-off-by: Oliver Steffen <osteffen@redhat.com>

This and next patch LGTM...

> ---
>  grub-core/commands/probe.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/grub-core/commands/probe.c b/grub-core/commands/probe.c
> index 9a80ea54f..3fe50d11e 100644
> --- a/grub-core/commands/probe.c
> +++ b/grub-core/commands/probe.c
> @@ -104,7 +104,7 @@ grub_cmd_probe (grub_extcmd_context_t ctxt, int argc, char **args)
>    if (state[6].set)
>      {
>        /* AAAABBBB-CCCC-DDDD-EEEE-FFFFFFFFFFFF + null terminator */
> -      char val[37] = "none";
> +      char val[GRUB_GPT_GUID_STR_LEN + 1] = "none";

Good! You are doing what I asked for in the reply earlier... :-)

>        if (dev->disk && dev->disk->partition)
>  	{
>  	  struct grub_partition *p = dev->disk->partition;
> @@ -130,14 +130,7 @@ grub_cmd_probe (grub_extcmd_context_t ctxt, int argc, char **args)
>  		  return grub_errno;
>  		}
>  	      guid = &entry.guid;
> -	      grub_snprintf (val, sizeof(val),
> -			     "%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]);
> +        grub_gpt_part_guid_snprint (val, sizeof(val), guid);

Daniel