[PATCH 0/2] *** SUBJECT HERE ***

[PATCH 0/2] *** SUBJECT HERE ***

[Colin Watson]

Some UEFI firmware is easily provoked into running out of space in its
variable storage.  This is usually due to certain kernel drivers (e.g.
pstore), but regardless of the cause it can cause grub-install to fail
because it currently asks efibootmgr to delete and re-add entries, and
the deletion often doesn't result in an immediate garbage collection.
Writing variables frequently also increases wear on the NVRAM which may
have limited write cycles.  For these reasons, it's desirable to find a
way to minimise writes while still allowing grub-install to ensure that
a suitable boot entry exists.

This short patch series does so by using the efivar and efiboot
libraries directly.

Colin Watson (2):
  Add %X to grub_vsnprintf_real and friends
  Minimise writes to EFI variable storage

 INSTALL                         |   5 +
 Makefile.util.def               |  20 ++
 configure.ac                    |  12 +
 grub-core/kern/misc.c           |  10 +-
 grub-core/osdep/efivar.c        |   3 +
 grub-core/osdep/unix/efivar.c   | 503 ++++++++++++++++++++++++++++++++
 grub-core/osdep/unix/platform.c | 100 +------
 include/grub/util/install.h     |   5 +
 util/grub-install.c             |   4 +-
 9 files changed, 565 insertions(+), 97 deletions(-)
 create mode 100644 grub-core/osdep/efivar.c
 create mode 100644 grub-core/osdep/unix/efivar.c

-- 
2.17.1

[PATCH 1/2] Add %X to grub_vsnprintf_real and friends

[Colin Watson]

This is needed for UEFI Boot* variables, which the standard says are
named using upper-case hexadecimal.
---
 grub-core/kern/misc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 3b633d51f..73f8e0e9e 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -588,7 +588,7 @@ grub_divmod64 (grub_uint64_t n, grub_uint64_t d, grub_uint64_t *r)
 static inline char *
 grub_lltoa (char *str, int c, unsigned long long n)
 {
-  unsigned base = (c == 'x') ? 16 : 10;
+  unsigned base = (c == 'x' || c == 'X') ? 16 : 10;
   char *p;
 
   if ((long long) n < 0 && c == 'd')
@@ -603,7 +603,10 @@ grub_lltoa (char *str, int c, unsigned long long n)
     do
       {
 	unsigned d = (unsigned) (n & 0xf);
-	*p++ = (d > 9) ? d + 'a' - 10 : d + '0';
+	*p = (d > 9) ? d + 'a' - 10 : d + '0';
+	if (c == 'X')
+	  *p = grub_toupper (*p);
+	p++;
       }
     while (n >>= 4);
   else
@@ -676,6 +679,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
 	{
 	case 'p':
 	case 'x':
+	case 'X':
 	case 'u':
 	case 'd':
 	case 'c':
@@ -762,6 +766,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
       switch (c)
 	{
 	case 'x':
+	case 'X':
 	case 'u':
 	  args->ptr[curn].type = UNSIGNED_INT + longfmt;
 	  break;
@@ -900,6 +905,7 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
 	  c = 'x';
 	  /* Fall through. */
 	case 'x':
+	case 'X':
 	case 'u':
 	case 'd':
 	  {
-- 
2.17.1

Re: [PATCH 1/2] Add %X to grub_vsnprintf_real and friends

[Daniel Kiper]

On Mon, Mar 11, 2019 at 03:05:19PM +0000, Colin Watson wrote:
> This is needed for UEFI Boot* variables, which the standard says are
> named using upper-case hexadecimal.

Missing SOB. I assume that I can add yours. If yes then
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH 1/2] Add %X to grub_vsnprintf_real and friends

[Colin Watson]

On Tue, Mar 12, 2019 at 01:19:09PM +0100, Daniel Kiper wrote:
> On Mon, Mar 11, 2019 at 03:05:19PM +0000, Colin Watson wrote:
> > This is needed for UEFI Boot* variables, which the standard says are
> > named using upper-case hexadecimal.
> 
> Missing SOB. I assume that I can add yours. If yes then
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Sorry, yes - please do.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 1/2] Add %X to grub_vsnprintf_real and friends

[Steve McIntyre]

On Mon, Mar 11, 2019 at 03:05:19PM +0000, Colin Watson wrote:
>This is needed for UEFI Boot* variables, which the standard says are
>named using upper-case hexadecimal.
>---
> grub-core/kern/misc.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
>index 3b633d51f..73f8e0e9e 100644
>--- a/grub-core/kern/misc.c
>+++ b/grub-core/kern/misc.c
>@@ -588,7 +588,7 @@ grub_divmod64 (grub_uint64_t n, grub_uint64_t d, grub_uint64_t *r)
> static inline char *
> grub_lltoa (char *str, int c, unsigned long long n)
> {
>-  unsigned base = (c == 'x') ? 16 : 10;
>+  unsigned base = (c == 'x' || c == 'X') ? 16 : 10;
>   char *p;
> 
>   if ((long long) n < 0 && c == 'd')
>@@ -603,7 +603,10 @@ grub_lltoa (char *str, int c, unsigned long long n)
>     do
>       {
> 	unsigned d = (unsigned) (n & 0xf);
>-	*p++ = (d > 9) ? d + 'a' - 10 : d + '0';
>+	*p = (d > 9) ? d + 'a' - 10 : d + '0';
>+	if (c == 'X')
>+	  *p = grub_toupper (*p);
>+	p++;

I'd be more tempted to simply do the upper-case stuff using

        if (c == 'x')
          *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
        else
          *p++ = (d > 9) ? d + 'A' - 10 : d + '0';
        fi

rather than the call out to grub_toupper(), but that's just minor
nit-picking on style.

>       }
>     while (n >>= 4);
>   else
>@@ -676,6 +679,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
> 	{
> 	case 'p':
> 	case 'x':
>+	case 'X':
> 	case 'u':
> 	case 'd':
> 	case 'c':
>@@ -762,6 +766,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
>       switch (c)
> 	{
> 	case 'x':
>+	case 'X':
> 	case 'u':
> 	  args->ptr[curn].type = UNSIGNED_INT + longfmt;
> 	  break;
>@@ -900,6 +905,7 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
> 	  c = 'x';
> 	  /* Fall through. */
> 	case 'x':
>+	case 'X':
> 	case 'u':
> 	case 'd':
> 	  {
>-- 
>2.17.1

Reviewed-by: Steve McIntyre <93sam@debian.org>

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
"Because heaters aren't purple!" -- Catherine Pitt

[PATCH 2/2] Minimise writes to EFI variable storage

[Colin Watson]

Some UEFI firmware is easily provoked into running out of space in its
variable storage.  This is usually due to certain kernel drivers (e.g.
pstore), but regardless of the cause it can cause grub-install to fail
because it currently asks efibootmgr to delete and re-add entries, and
the deletion often doesn't result in an immediate garbage collection.
Writing variables frequently also increases wear on the NVRAM which may
have limited write cycles.  For these reasons, it's desirable to find a
way to minimise writes while still allowing grub-install to ensure that
a suitable boot entry exists.

Unfortunately, efibootmgr doesn't offer an interface that would let
grub-install do this.  It doesn't in general make very much effort to
minimise writes; it doesn't allow modifying an existing Boot* variable
entry, except in certain limited ways; and current versions don't have a
way to export the expected variable data so that grub-install can
compare it to the current data.  While it would be possible (and perhaps
desirable?) to add at least some of this to efibootmgr, that would still
leave the problem that there isn't a good upstreamable way for
grub-install to guarantee that it has a new enough version of
efibootmgr.  In any case, it's cumbersome and slow for grub-install to
have to fork efibootmgr to get things done.

Fortunately, a few years ago Peter Jones helpfully factored out a
substantial part of efibootmgr to the efivar and efiboot libraries, and
so it's now possible to have grub-install use those directly.  We still
have to use some code from efibootmgr, but much less than would
previously have been necessary.

grub-install now reuses existing boot entries where possible, and avoids
writing to variables when the new contents are the same as the old
contents.  In the common upgrade case where nothing needs to change, it
no longer writes to NVRAM at all.  It's also now slightly faster, since
using libefivar is faster than forking efibootmgr.

Fixes Debian bug #891434.

Signed-off-by: Colin Watson <cjwatson@ubuntu.com>
---
 INSTALL                         |   5 +
 Makefile.util.def               |  20 ++
 configure.ac                    |  12 +
 grub-core/osdep/efivar.c        |   3 +
 grub-core/osdep/unix/efivar.c   | 503 ++++++++++++++++++++++++++++++++
 grub-core/osdep/unix/platform.c | 100 +------
 include/grub/util/install.h     |   5 +
 util/grub-install.c             |   4 +-
 8 files changed, 557 insertions(+), 95 deletions(-)
 create mode 100644 grub-core/osdep/efivar.c
 create mode 100644 grub-core/osdep/unix/efivar.c

diff --git a/INSTALL b/INSTALL
index 8acb40902..342c158e9 100644
--- a/INSTALL
+++ b/INSTALL
@@ -41,6 +41,11 @@ configuring the GRUB.
 * Other standard GNU/Unix tools
 * a libc with large file support (e.g. glibc 2.1 or later)
 
+On Unix-based systems, you also need:
+
+* libefivar (recommended)
+* libefiboot (recommended; your OS may ship this together with libefivar)
+
 On GNU/Linux, you also need:
 
 * libdevmapper 1.02.34 or later (recommended)
diff --git a/Makefile.util.def b/Makefile.util.def
index 969d32f00..60ec4d593 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -535,6 +535,8 @@ program = {
   common = grub-core/osdep/compress.c;
   extra_dist = grub-core/osdep/unix/compress.c;
   extra_dist = grub-core/osdep/basic/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -548,12 +550,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 
   condition = COND_HAVE_EXEC;
 };
@@ -582,6 +587,8 @@ program = {
   extra_dist = grub-core/osdep/basic/no_platform.c;
   extra_dist = grub-core/osdep/unix/platform.c;
   common = grub-core/osdep/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -595,12 +602,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 };
 
 program = {
@@ -622,6 +632,8 @@ program = {
   common = grub-core/osdep/platform.c;
   common = grub-core/osdep/platform_unix.c;
   common = grub-core/osdep/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -634,12 +646,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 };
 
 program = {
@@ -661,6 +676,8 @@ program = {
   common = grub-core/osdep/platform.c;
   common = grub-core/osdep/platform_unix.c;
   common = grub-core/osdep/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -670,12 +687,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 };
 
 script = {
diff --git a/configure.ac b/configure.ac
index 05d040cd6..2add2a092 100644
--- a/configure.ac
+++ b/configure.ac
@@ -443,6 +443,18 @@ AC_CHECK_HEADER([util.h], [
 ])
 AC_SUBST([LIBUTIL])
 
+case "$host_os" in
+  cygwin | windows* | mingw32* | aros*)
+    ;;
+  *)
+    # For setting EFI variables in grub-install.
+    PKG_CHECK_MODULES([EFIVAR], [efivar efiboot], [
+      AC_DEFINE([HAVE_EFIVAR], [1],
+                [Define to 1 if you have the efivar and efiboot libraries.])
+    ], [:])
+    ;;
+esac
+
 AC_CACHE_CHECK([whether -Wtrampolines work], [grub_cv_host_cc_wtrampolines], [
   SAVED_CFLAGS="$CFLAGS"
   CFLAGS="$HOST_CFLAGS -Wtrampolines -Werror"
diff --git a/grub-core/osdep/efivar.c b/grub-core/osdep/efivar.c
new file mode 100644
index 000000000..d2750e252
--- /dev/null
+++ b/grub-core/osdep/efivar.c
@@ -0,0 +1,3 @@
+#if !defined (__MINGW32__) && !defined (__CYGWIN__) && !defined (__AROS__)
+#include "unix/efivar.c"
+#endif
diff --git a/grub-core/osdep/unix/efivar.c b/grub-core/osdep/unix/efivar.c
new file mode 100644
index 000000000..1df3e38f1
--- /dev/null
+++ b/grub-core/osdep/unix/efivar.c
@@ -0,0 +1,503 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2013,2019 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/>.
+ */
+
+/* Contains portions derived from efibootmgr, licensed as follows:
+ *
+ *  Copyright (C) 2001-2004 Dell, Inc. <Matt_Domsch@dell.com>
+ *  Copyright 2015-2016 Red Hat, Inc. <pjones@redhat.com>
+ *
+ *  This program 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 2 of the License, or
+ *  (at your option) any later version.
+ */
+
+#include <config.h>
+
+#ifdef HAVE_EFIVAR
+
+#include <grub/util/install.h>
+#include <grub/emu/hostdisk.h>
+#include <grub/util/misc.h>
+#include <grub/list.h>
+#include <grub/misc.h>
+#include <grub/emu/exec.h>
+#include <sys/types.h>
+#include <ctype.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <efiboot.h>
+#include <efivar.h>
+
+struct efi_variable {
+  struct efi_variable *next;
+  struct efi_variable **prev;
+  char *name;
+  efi_guid_t guid;
+  uint8_t *data;
+  size_t data_size;
+  uint32_t attributes;
+  int num;
+};
+
+/* Boot option attributes.  */
+#define LOAD_OPTION_ACTIVE 0x00000001
+
+/* GUIDs.  */
+#define BLKX_UNKNOWN_GUID \
+  EFI_GUID (0x47c7b225, 0xc42a, 0x11d2, 0x8e57, 0x00, 0xa0, 0xc9, 0x69, \
+	    0x72, 0x3b)
+
+/* Log all errors recorded by libefivar/libefiboot.  */
+static void
+show_efi_errors (void)
+{
+  int i;
+  int saved_errno = errno;
+
+  for (i = 0; ; ++i)
+    {
+      char *filename, *function, *message = NULL;
+      int line, error = 0, rc;
+
+      rc = efi_error_get (i, &filename, &function, &line, &message, &error);
+      if (rc < 0)
+	/* Give up.  The caller is going to log an error anyway.  */
+	break;
+      if (rc == 0)
+	/* No more errors.  */
+	break;
+      grub_util_warn ("%s: %s: %s", function, message, strerror (error));
+    }
+
+  efi_error_clear ();
+  errno = saved_errno;
+}
+
+static struct efi_variable *
+new_efi_variable (void)
+{
+  struct efi_variable *new = xmalloc (sizeof (*new));
+  memset (new, 0, sizeof (*new));
+  return new;
+}
+
+static struct efi_variable *
+new_boot_variable (void)
+{
+  struct efi_variable *new = new_efi_variable ();
+  new->guid = EFI_GLOBAL_GUID;
+  new->attributes = EFI_VARIABLE_NON_VOLATILE |
+		    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+		    EFI_VARIABLE_RUNTIME_ACCESS;
+  return new;
+}
+
+static void
+free_efi_variable (struct efi_variable *entry)
+{
+  if (entry)
+    {
+      free (entry->name);
+      free (entry->data);
+      free (entry);
+    }
+}
+
+static int
+read_efi_variable (const char *name, struct efi_variable **entry)
+{
+  struct efi_variable *new = new_efi_variable ();
+  int rc;
+
+  rc = efi_get_variable (EFI_GLOBAL_GUID, name,
+			 &new->data, &new->data_size, &new->attributes);
+  if (rc < 0)
+    {
+      free (new);
+      new = NULL;
+    }
+
+  if (new)
+    {
+      /* Latest Apple firmware sets the high bit which appears invalid
+	 to the Linux kernel if we write it back, so let's zero it out if it
+	 is set since it would be invalid to set it anyway.  */
+      new->attributes = new->attributes & ~(1 << 31);
+
+      new->name = xstrdup (name);
+      new->guid = EFI_GLOBAL_GUID;
+    }
+
+  *entry = new;
+  return rc;
+}
+
+/* Set an EFI variable, but only if it differs from the current value.
+   Some firmware implementations are liable to fill up flash space if we set
+   variables unnecessarily, so try to keep write activity to a minimum. */
+static int
+set_efi_variable (const char *name, struct efi_variable *entry)
+{
+  struct efi_variable *old = NULL;
+  int rc = 0;
+
+  read_efi_variable (name, &old);
+  efi_error_clear ();
+  if (old && old->attributes == entry->attributes &&
+      old->data_size == entry->data_size &&
+      memcmp (old->data, entry->data, entry->data_size) == 0)
+    grub_util_info ("skipping unnecessary update of EFI variable %s", name);
+  else
+    {
+      rc = efi_set_variable (EFI_GLOBAL_GUID, name,
+			     entry->data, entry->data_size, entry->attributes,
+			     0644);
+      if (rc < 0)
+	grub_util_warn (_("Cannot set EFI variable %s"), name);
+    }
+  free_efi_variable (old);
+  return rc;
+}
+
+static int
+cmpvarbyname (const void *p1, const void *p2)
+{
+  const struct efi_variable *var1 = *(const struct efi_variable **)p1;
+  const struct efi_variable *var2 = *(const struct efi_variable **)p2;
+  return strcmp (var1->name, var2->name);
+}
+
+static int
+read_boot_variables (struct efi_variable **varlist)
+{
+  int rc;
+  efi_guid_t *guid = NULL;
+  char *name = NULL;
+  struct efi_variable **newlist = NULL;
+  int nentries = 0;
+  int i;
+
+  while ((rc = efi_get_next_variable_name (&guid, &name)) > 0)
+    {
+      const char *snum = name + sizeof ("Boot") - 1;
+      struct efi_variable *var = NULL;
+      unsigned int num;
+
+      if (memcmp (guid, &efi_guid_global, sizeof (efi_guid_global)) != 0 ||
+	  strncmp (name, "Boot", sizeof ("Boot") - 1) != 0 ||
+	  !grub_isxdigit (snum[0]) || !grub_isxdigit (snum[1]) ||
+	  !grub_isxdigit (snum[2]) || !grub_isxdigit (snum[3]))
+	continue;
+
+      rc = read_efi_variable (name, &var);
+      if (rc < 0)
+	break;
+
+      if (sscanf (var->name, "Boot%04X-%*s", &num) == 1 && num < 65536)
+	var->num = num;
+
+      newlist = xrealloc (newlist, (++nentries) * sizeof (*newlist));
+      newlist[nentries - 1] = var;
+    }
+  if (rc == 0 && newlist)
+    {
+      qsort (newlist, nentries, sizeof (*newlist), cmpvarbyname);
+      for (i = nentries - 1; i >= 0; --i)
+	grub_list_push (GRUB_AS_LIST_P (varlist), GRUB_AS_LIST (newlist[i]));
+    }
+  else if (newlist)
+    {
+      for (i = 0; i < nentries; ++i)
+	free (newlist[i]);
+      free (newlist);
+    }
+  return rc;
+}
+
+static void
+remove_from_boot_order (struct efi_variable *order, uint16_t num)
+{
+  uint16_t *data;
+  unsigned int old_i, new_i;
+
+  /* We've got an array (in order->data) of the order.  Squeeze out any
+     instance of the entry we're deleting by shifting the remainder down.  */
+  data = (uint16_t *) order->data;
+
+  for (old_i = 0, new_i = 0;
+       old_i < order->data_size / sizeof (uint16_t);
+       ++old_i)
+    {
+      if (data[old_i] != num) {
+	if (new_i != old_i)
+	  data[new_i] = data[old_i];
+	new_i++;
+      }
+    }
+
+  order->data_size = sizeof (data[0]) * new_i;
+}
+
+static void
+add_to_boot_order (struct efi_variable *order, uint16_t num)
+{
+  uint16_t *data;
+  int i;
+  size_t new_data_size;
+  uint16_t *new_data;
+
+  /* Check whether this entry is already in the boot order.  If it is, leave
+     it alone.  */
+  data = (uint16_t *) order->data;
+  for (i = 0; i < order->data_size / sizeof (uint16_t); ++i)
+    if (data[i] == num)
+      return;
+
+  new_data_size = order->data_size + sizeof (uint16_t);
+  new_data = xmalloc (new_data_size);
+  new_data[0] = num;
+  memcpy (new_data + 1, order->data, order->data_size);
+  free (order->data);
+  order->data = (uint8_t *) new_data;
+  order->data_size = new_data_size;
+}
+
+static int
+find_free_boot_num (struct efi_variable *entries)
+{
+  int num_vars = 0, i;
+  struct efi_variable *entry;
+
+  FOR_LIST_ELEMENTS (entry, entries)
+    ++num_vars;
+
+  if (num_vars == 0)
+    return 0;
+
+  /* O(n^2), but n is small and this is easy. */
+  for (i = 0; i < num_vars; ++i)
+    {
+      int found = 0;
+      FOR_LIST_ELEMENTS (entry, entries)
+	{
+	  if (entry->num == i)
+	    {
+	      found = 1;
+	      break;
+	    }
+	}
+      if (!found)
+	return i;
+    }
+
+  return i;
+}
+
+static int
+get_edd_version (void)
+{
+  efi_guid_t blkx_guid = BLKX_UNKNOWN_GUID;
+  uint8_t *data = NULL;
+  size_t data_size = 0;
+  uint32_t attributes;
+  efidp_header *path;
+  int rc;
+
+  rc = efi_get_variable (blkx_guid, "blk0", &data, &data_size, &attributes);
+  if (rc < 0)
+    return rc;
+
+  path = (efidp_header *) data;
+  if (path->type == 2 && path->subtype == 1)
+    return 3;
+  return 1;
+}
+
+static struct efi_variable *
+make_boot_variable (int num, const char *disk, int part, const char *loader,
+		    const char *label)
+{
+  struct efi_variable *entry = new_boot_variable ();
+  uint32_t options;
+  uint32_t edd10_devicenum;
+  ssize_t dp_needed, loadopt_needed;
+  efidp dp = NULL;
+
+  options = EFIBOOT_ABBREV_HD;
+  switch (get_edd_version ()) {
+    case 1:
+      options = EFIBOOT_ABBREV_EDD10;
+      break;
+    case 3:
+      options = EFIBOOT_ABBREV_NONE;
+      break;
+  }
+
+  /* This may not be the right disk; but it's probably only an issue on very
+     old hardware anyway. */
+  edd10_devicenum = 0x80;
+
+  dp_needed = efi_generate_file_device_path_from_esp (NULL, 0, disk, part,
+						      loader, options,
+						      edd10_devicenum);
+  if (dp_needed < 0)
+    goto err;
+
+  dp = xmalloc (dp_needed);
+  dp_needed = efi_generate_file_device_path_from_esp ((uint8_t *) dp,
+						      dp_needed, disk, part,
+						      loader, options,
+						      edd10_devicenum);
+  if (dp_needed < 0)
+    goto err;
+
+  loadopt_needed = efi_loadopt_create (NULL, 0, LOAD_OPTION_ACTIVE,
+				       dp, dp_needed, (unsigned char *) label,
+				       NULL, 0);
+  if (loadopt_needed < 0)
+    goto err;
+  entry->data_size = loadopt_needed;
+  entry->data = xmalloc (entry->data_size);
+  loadopt_needed = efi_loadopt_create (entry->data, entry->data_size,
+				       LOAD_OPTION_ACTIVE, dp, dp_needed,
+				       (unsigned char *) label, NULL, 0);
+  if (loadopt_needed < 0)
+    goto err;
+
+  entry->name = xasprintf ("Boot%04X", num);
+  entry->num = num;
+
+  return entry;
+
+err:
+  free_efi_variable (entry);
+  free (dp);
+  return NULL;
+}
+
+int
+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
+				  const char *efifile_path,
+				  const char *efi_distributor)
+{
+  const char *efidir_disk;
+  int efidir_part;
+  struct efi_variable *entries = NULL, *entry;
+  struct efi_variable *order;
+  int entry_num = -1;
+  int rc;
+
+  efidir_disk = grub_util_biosdisk_get_osdev (efidir_grub_dev->disk);
+  efidir_part = efidir_grub_dev->disk->partition ? efidir_grub_dev->disk->partition->number + 1 : 1;
+
+#ifdef __linux__
+  /*
+   * Linux uses efivarfs (mounted on /sys/firmware/efi/efivars) to access the
+   * EFI variable store. Some legacy systems may still use the deprecated
+   * efivars interface (accessed through /sys/firmware/efi/vars). Where both
+   * are present, libefivar will use the former in preference, so attempting
+   * to load efivars will not interfere with later operations.
+   */
+  grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", NULL },
+			       NULL, NULL, "/dev/null");
+#endif
+
+  if (!efi_variables_supported ())
+    {
+      grub_util_warn ("%s",
+		      _("EFI variables are not supported on this system."));
+      /* Let the user continue.  Perhaps they can still arrange to boot GRUB
+         manually.  */
+      return 0;
+    }
+
+  rc = read_boot_variables (&entries);
+  if (rc < 0)
+    {
+      grub_util_warn ("%s", _("Cannot read EFI Boot* variables"));
+      goto err;
+    }
+  rc = read_efi_variable ("BootOrder", &order);
+  if (rc < 0)
+    {
+      order = new_boot_variable ();
+      order->name = xstrdup ("BootOrder");
+      efi_error_clear ();
+    }
+
+  /* Delete old entries from the same distributor.  */
+  FOR_LIST_ELEMENTS (entry, entries)
+    {
+      efi_load_option *load_option = (efi_load_option *) entry->data;
+      const char *label;
+
+      if (entry->num < 0)
+	continue;
+      label = (const char *) efi_loadopt_desc (load_option, entry->data_size);
+      if (strcasecmp (label, efi_distributor) != 0)
+	continue;
+
+      /* To avoid problems with some firmware implementations, reuse the first
+         matching variable we find rather than deleting and recreating it.  */
+      if (entry_num == -1)
+	entry_num = entry->num;
+      else
+	{
+	  grub_util_info ("deleting superfluous EFI variable %s (%s)",
+			  entry->name, label);
+	  rc = efi_del_variable (EFI_GLOBAL_GUID, entry->name);
+	  if (rc < 0)
+	    {
+	      grub_util_warn (_("Cannot delete EFI variable %s"), entry->name);
+	      goto err;
+	    }
+	}
+
+      remove_from_boot_order (order, (uint16_t) entry->num);
+    }
+
+  if (entry_num == -1)
+    entry_num = find_free_boot_num (entries);
+  entry = make_boot_variable (entry_num, efidir_disk, efidir_part,
+			      efifile_path, efi_distributor);
+  if (!entry)
+    goto err;
+
+  grub_util_info ("setting EFI variable %s", entry->name);
+  rc = set_efi_variable (entry->name, entry);
+  if (rc < 0)
+    goto err;
+
+  add_to_boot_order (order, (uint16_t) entry_num);
+
+  grub_util_info ("setting EFI variable BootOrder");
+  rc = set_efi_variable ("BootOrder", order);
+  if (rc < 0)
+    goto err;
+
+  return 0;
+
+err:
+  show_efi_errors ();
+  return errno;
+}
+
+#endif /* HAVE_EFIVAR */
diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
index 55b8f4016..9d4530a38 100644
--- a/grub-core/osdep/unix/platform.c
+++ b/grub-core/osdep/unix/platform.c
@@ -19,15 +19,12 @@
 #include <config.h>
 
 #include <grub/util/install.h>
-#include <grub/emu/hostdisk.h>
 #include <grub/util/misc.h>
 #include <grub/misc.h>
 #include <grub/i18n.h>
 #include <grub/emu/exec.h>
 #include <sys/types.h>
-#include <dirent.h>
 #include <string.h>
-#include <errno.h>
 
 static char *
 get_ofpathname (const char *dev)
@@ -78,102 +75,19 @@ get_ofpathname (const char *dev)
 		   dev);
 }
 
-static int
-grub_install_remove_efi_entries_by_distributor (const char *efi_distributor)
-{
-  int fd;
-  pid_t pid = grub_util_exec_pipe ((const char * []){ "efibootmgr", NULL }, &fd);
-  char *line = NULL;
-  size_t len = 0;
-  int rc = 0;
-
-  if (!pid)
-    {
-      grub_util_warn (_("Unable to open stream from %s: %s"),
-		      "efibootmgr", strerror (errno));
-      return errno;
-    }
-
-  FILE *fp = fdopen (fd, "r");
-  if (!fp)
-    {
-      grub_util_warn (_("Unable to open stream from %s: %s"),
-		      "efibootmgr", strerror (errno));
-      return errno;
-    }
-
-  line = xmalloc (80);
-  len = 80;
-  while (1)
-    {
-      int ret;
-      char *bootnum;
-      ret = getline (&line, &len, fp);
-      if (ret == -1)
-	break;
-      if (grub_memcmp (line, "Boot", sizeof ("Boot") - 1) != 0
-	  || line[sizeof ("Boot") - 1] < '0'
-	  || line[sizeof ("Boot") - 1] > '9')
-	continue;
-      if (!strcasestr (line, efi_distributor))
-	continue;
-      bootnum = line + sizeof ("Boot") - 1;
-      bootnum[4] = '\0';
-      if (!verbosity)
-	rc = grub_util_exec ((const char * []){ "efibootmgr", "-q",
-	      "-b", bootnum,  "-B", NULL });
-      else
-	rc = grub_util_exec ((const char * []){ "efibootmgr",
-	      "-b", bootnum, "-B", NULL });
-    }
-
-  free (line);
-  return rc;
-}
-
 int
 grub_install_register_efi (grub_device_t efidir_grub_dev,
 			   const char *efifile_path,
 			   const char *efi_distributor)
 {
-  const char * efidir_disk;
-  int efidir_part;
-  int ret;
-  efidir_disk = grub_util_biosdisk_get_osdev (efidir_grub_dev->disk);
-  efidir_part = efidir_grub_dev->disk->partition ? efidir_grub_dev->disk->partition->number + 1 : 1;
-
-  if (grub_util_exec_redirect_null ((const char * []){ "efibootmgr", "--version", NULL }))
-    {
-      /* TRANSLATORS: This message is shown when required executable `%s'
-	 isn't found.  */
-      grub_util_error (_("%s: not found"), "efibootmgr");
-    }
-
-  /* On Linux, we need the efivars kernel modules.  */
-#ifdef __linux__
-  grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL });
+#ifdef HAVE_EFIVAR
+  return grub_install_efivar_register_efi (efidir_grub_dev, efifile_path,
+					   efi_distributor);
+#else
+  grub_util_error ("%s",
+		   _("GRUB was not built with efivar support; "
+		     "cannot register EFI boot entry"));
 #endif
-  /* Delete old entries from the same distributor.  */
-  ret = grub_install_remove_efi_entries_by_distributor (efi_distributor);
-  if (ret)
-    return ret;
-
-  char *efidir_part_str = xasprintf ("%d", efidir_part);
-
-  if (!verbosity)
-    ret = grub_util_exec ((const char * []){ "efibootmgr", "-q",
-	  "-c", "-d", efidir_disk,
-	  "-p", efidir_part_str, "-w",
-	  "-L", efi_distributor, "-l", 
-	  efifile_path, NULL });
-  else
-    ret = grub_util_exec ((const char * []){ "efibootmgr",
-	  "-c", "-d", efidir_disk,
-	  "-p", efidir_part_str, "-w",
-	  "-L", efi_distributor, "-l", 
-	  efifile_path, NULL });
-  free (efidir_part_str);
-  return ret;
 }
 
 void
diff --git a/include/grub/util/install.h b/include/grub/util/install.h
index 2631b1074..dcad16ac5 100644
--- a/include/grub/util/install.h
+++ b/include/grub/util/install.h
@@ -216,6 +216,11 @@ grub_install_get_default_arm_platform (void);
 const char *
 grub_install_get_default_x86_platform (void);
 
+int
+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
+				  const char *efifile_path,
+				  const char *efi_distributor);
+
 int
 grub_install_register_efi (grub_device_t efidir_grub_dev,
 			   const char *efifile_path,
diff --git a/util/grub-install.c b/util/grub-install.c
index 264f9ecdc..a61df056e 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -1885,7 +1885,7 @@ main (int argc, char *argv[])
 					       "\\System\\Library\\CoreServices",
 					       efi_distributor);
 	      if (ret)
-	        grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
+	        grub_util_error (_("failed to register the EFI boot entry: %s"),
 				 strerror (ret));
 	    }
 
@@ -1929,7 +1929,7 @@ main (int argc, char *argv[])
 	  ret = grub_install_register_efi (efidir_grub_dev,
 					   efifile_path, efi_distributor);
 	  if (ret)
-	    grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
+	    grub_util_error (_("failed to register the EFI boot entry: %s"),
 			     strerror (ret));
 	}
       break;
-- 
2.17.1

Re: [PATCH 2/2] Minimise writes to EFI variable storage

[Steve McIntyre]

First of all, thanks very much for doing this!

(For other people: I came up with a previous way of doing this years
ago, using efibootmgr to ask about the state of a variable before
maybe deleting and re-setting it.)

On Mon, Mar 11, 2019 at 03:05:46PM +0000, Colin Watson wrote:
>Some UEFI firmware is easily provoked into running out of space in its
>variable storage.  This is usually due to certain kernel drivers (e.g.
>pstore), but regardless of the cause it can cause grub-install to fail
>because it currently asks efibootmgr to delete and re-add entries, and
>the deletion often doesn't result in an immediate garbage collection.
>Writing variables frequently also increases wear on the NVRAM which may
>have limited write cycles.  For these reasons, it's desirable to find a
>way to minimise writes while still allowing grub-install to ensure that
>a suitable boot entry exists.
>
>Unfortunately, efibootmgr doesn't offer an interface that would let
>grub-install do this.  It doesn't in general make very much effort to
>minimise writes; it doesn't allow modifying an existing Boot* variable
>entry, except in certain limited ways; and current versions don't have a
>way to export the expected variable data so that grub-install can
>compare it to the current data.  While it would be possible (and perhaps
>desirable?) to add at least some of this to efibootmgr, that would still
>leave the problem that there isn't a good upstreamable way for
>grub-install to guarantee that it has a new enough version of
>efibootmgr.  In any case, it's cumbersome and slow for grub-install to
>have to fork efibootmgr to get things done.
>
>Fortunately, a few years ago Peter Jones helpfully factored out a
>substantial part of efibootmgr to the efivar and efiboot libraries, and
>so it's now possible to have grub-install use those directly.  We still
>have to use some code from efibootmgr, but much less than would
>previously have been necessary.
>
>grub-install now reuses existing boot entries where possible, and avoids
>writing to variables when the new contents are the same as the old
>contents.  In the common upgrade case where nothing needs to change, it
>no longer writes to NVRAM at all.  It's also now slightly faster, since
>using libefivar is faster than forking efibootmgr.

Awesome!

Some detailed review stuff inline below, with (maybe?) one error noted
in read_efi_variable().

I've built this code but not tested it directly on real hardware yet.

>Fixes Debian bug #891434.
>
>Signed-off-by: Colin Watson <cjwatson@ubuntu.com>
>---
> INSTALL                         |   5 +
> Makefile.util.def               |  20 ++
> configure.ac                    |  12 +
> grub-core/osdep/efivar.c        |   3 +
> grub-core/osdep/unix/efivar.c   | 503 ++++++++++++++++++++++++++++++++
> grub-core/osdep/unix/platform.c | 100 +------
> include/grub/util/install.h     |   5 +
> util/grub-install.c             |   4 +-
> 8 files changed, 557 insertions(+), 95 deletions(-)
> create mode 100644 grub-core/osdep/efivar.c
> create mode 100644 grub-core/osdep/unix/efivar.c

...

>diff --git a/grub-core/osdep/efivar.c b/grub-core/osdep/efivar.c
>new file mode 100644
>index 000000000..d2750e252
>--- /dev/null
>+++ b/grub-core/osdep/efivar.c
>@@ -0,0 +1,3 @@
>+#if !defined (__MINGW32__) && !defined (__CYGWIN__) && !defined (__AROS__)
>+#include "unix/efivar.c"
>+#endif
>diff --git a/grub-core/osdep/unix/efivar.c b/grub-core/osdep/unix/efivar.c
>new file mode 100644
>index 000000000..1df3e38f1
>--- /dev/null
>+++ b/grub-core/osdep/unix/efivar.c
>@@ -0,0 +1,503 @@
>+/*
>+ *  GRUB  --  GRand Unified Bootloader
>+ *  Copyright (C) 2013,2019 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/>.
>+ */
>+
>+/* Contains portions derived from efibootmgr, licensed as follows:
>+ *
>+ *  Copyright (C) 2001-2004 Dell, Inc. <Matt_Domsch@dell.com>
>+ *  Copyright 2015-2016 Red Hat, Inc. <pjones@redhat.com>
>+ *
>+ *  This program 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 2 of the License, or
>+ *  (at your option) any later version.
>+ */
>+
>+#include <config.h>
>+
>+#ifdef HAVE_EFIVAR
>+
>+#include <grub/util/install.h>
>+#include <grub/emu/hostdisk.h>
>+#include <grub/util/misc.h>
>+#include <grub/list.h>
>+#include <grub/misc.h>
>+#include <grub/emu/exec.h>
>+#include <sys/types.h>
>+#include <ctype.h>
>+#include <errno.h>
>+#include <stdlib.h>
>+#include <string.h>
>+
>+#include <efiboot.h>
>+#include <efivar.h>
>+
>+struct efi_variable {
>+  struct efi_variable *next;
>+  struct efi_variable **prev;
>+  char *name;
>+  efi_guid_t guid;
>+  uint8_t *data;
>+  size_t data_size;
>+  uint32_t attributes;
>+  int num;
>+};
>+
>+/* Boot option attributes.  */
>+#define LOAD_OPTION_ACTIVE 0x00000001
>+
>+/* GUIDs.  */
>+#define BLKX_UNKNOWN_GUID \
>+  EFI_GUID (0x47c7b225, 0xc42a, 0x11d2, 0x8e57, 0x00, 0xa0, 0xc9, 0x69, \
>+	    0x72, 0x3b)

Ugh. I'm assuming the mahic numbers here are not exposed usefully by
efivar or efiboot?

>+/* Log all errors recorded by libefivar/libefiboot.  */
>+static void
>+show_efi_errors (void)
>+{
>+  int i;
>+  int saved_errno = errno;
>+
>+  for (i = 0; ; ++i)
>+    {
>+      char *filename, *function, *message = NULL;
>+      int line, error = 0, rc;
>+
>+      rc = efi_error_get (i, &filename, &function, &line, &message, &error);
>+      if (rc < 0)
>+	/* Give up.  The caller is going to log an error anyway.  */
>+	break;
>+      if (rc == 0)
>+	/* No more errors.  */
>+	break;
>+      grub_util_warn ("%s: %s: %s", function, message, strerror (error));
>+    }
>+
>+  efi_error_clear ();
>+  errno = saved_errno;
>+}
>+
>+static struct efi_variable *
>+new_efi_variable (void)
>+{
>+  struct efi_variable *new = xmalloc (sizeof (*new));
>+  memset (new, 0, sizeof (*new));
>+  return new;
>+}
>+
>+static struct efi_variable *
>+new_boot_variable (void)
>+{
>+  struct efi_variable *new = new_efi_variable ();
>+  new->guid = EFI_GLOBAL_GUID;
>+  new->attributes = EFI_VARIABLE_NON_VOLATILE |
>+		    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>+		    EFI_VARIABLE_RUNTIME_ACCESS;
>+  return new;
>+}
>+
>+static void
>+free_efi_variable (struct efi_variable *entry)
>+{
>+  if (entry)
>+    {
>+      free (entry->name);
>+      free (entry->data);
>+      free (entry);
>+    }
>+}
>+
>+static int
>+read_efi_variable (const char *name, struct efi_variable **entry)
>+{
>+  struct efi_variable *new = new_efi_variable ();
>+  int rc;
>+
>+  rc = efi_get_variable (EFI_GLOBAL_GUID, name,
>+			 &new->data, &new->data_size, &new->attributes);
>+  if (rc < 0)
>+    {
>+      free (new);
>+      new = NULL;
>+    }

So new_efi_variable() is using xmalloc() so it's safe if the
allocation fails. But what happens if efi_get_variable() fails - do
you need to free all the members by calling free_efi_variable() rather
than simply free() here?

>+
>+  if (new)
>+    {
>+      /* Latest Apple firmware sets the high bit which appears invalid
>+	 to the Linux kernel if we write it back, so let's zero it out if it
>+	 is set since it would be invalid to set it anyway.  */
>+      new->attributes = new->attributes & ~(1 << 31);
>+
>+      new->name = xstrdup (name);
>+      new->guid = EFI_GLOBAL_GUID;
>+    }
>+
>+  *entry = new;
>+  return rc;
>+}
>+
>+/* Set an EFI variable, but only if it differs from the current value.
>+   Some firmware implementations are liable to fill up flash space if we set
>+   variables unnecessarily, so try to keep write activity to a minimum. */
>+static int
>+set_efi_variable (const char *name, struct efi_variable *entry)
>+{
>+  struct efi_variable *old = NULL;
>+  int rc = 0;
>+
>+  read_efi_variable (name, &old);
>+  efi_error_clear ();
>+  if (old && old->attributes == entry->attributes &&
>+      old->data_size == entry->data_size &&
>+      memcmp (old->data, entry->data, entry->data_size) == 0)
>+    grub_util_info ("skipping unnecessary update of EFI variable %s", name);
>+  else
>+    {
>+      rc = efi_set_variable (EFI_GLOBAL_GUID, name,
>+			     entry->data, entry->data_size, entry->attributes,
>+			     0644);
>+      if (rc < 0)
>+	grub_util_warn (_("Cannot set EFI variable %s"), name);
>+    }
>+  free_efi_variable (old);
>+  return rc;
>+}
>+
>+static int
>+cmpvarbyname (const void *p1, const void *p2)
>+{
>+  const struct efi_variable *var1 = *(const struct efi_variable **)p1;
>+  const struct efi_variable *var2 = *(const struct efi_variable **)p2;
>+  return strcmp (var1->name, var2->name);
>+}
>+
>+static int
>+read_boot_variables (struct efi_variable **varlist)
>+{
>+  int rc;
>+  efi_guid_t *guid = NULL;
>+  char *name = NULL;
>+  struct efi_variable **newlist = NULL;
>+  int nentries = 0;
>+  int i;
>+
>+  while ((rc = efi_get_next_variable_name (&guid, &name)) > 0)
>+    {
>+      const char *snum = name + sizeof ("Boot") - 1;
>+      struct efi_variable *var = NULL;
>+      unsigned int num;
>+
>+      if (memcmp (guid, &efi_guid_global, sizeof (efi_guid_global)) != 0 ||
>+	  strncmp (name, "Boot", sizeof ("Boot") - 1) != 0 ||
>+	  !grub_isxdigit (snum[0]) || !grub_isxdigit (snum[1]) ||
>+	  !grub_isxdigit (snum[2]) || !grub_isxdigit (snum[3]))
>+	continue;
>+
>+      rc = read_efi_variable (name, &var);
>+      if (rc < 0)
>+	break;
>+
>+      if (sscanf (var->name, "Boot%04X-%*s", &num) == 1 && num < 65536)
>+	var->num = num;
>+
>+      newlist = xrealloc (newlist, (++nentries) * sizeof (*newlist));
>+      newlist[nentries - 1] = var;
>+    }
>+  if (rc == 0 && newlist)
>+    {
>+      qsort (newlist, nentries, sizeof (*newlist), cmpvarbyname);
>+      for (i = nentries - 1; i >= 0; --i)
>+	grub_list_push (GRUB_AS_LIST_P (varlist), GRUB_AS_LIST (newlist[i]));
>+    }
>+  else if (newlist)
>+    {
>+      for (i = 0; i < nentries; ++i)
>+	free (newlist[i]);
>+      free (newlist);
>+    }
>+  return rc;
>+}
>+
>+static void
>+remove_from_boot_order (struct efi_variable *order, uint16_t num)
>+{
>+  uint16_t *data;
>+  unsigned int old_i, new_i;
>+
>+  /* We've got an array (in order->data) of the order.  Squeeze out any
>+     instance of the entry we're deleting by shifting the remainder down.  */
>+  data = (uint16_t *) order->data;
>+
>+  for (old_i = 0, new_i = 0;
>+       old_i < order->data_size / sizeof (uint16_t);
>+       ++old_i)
>+    {
>+      if (data[old_i] != num) {
>+	if (new_i != old_i)
>+	  data[new_i] = data[old_i];
>+	new_i++;
>+      }
>+    }
>+
>+  order->data_size = sizeof (data[0]) * new_i;
>+}
>+
>+static void
>+add_to_boot_order (struct efi_variable *order, uint16_t num)
>+{
>+  uint16_t *data;
>+  int i;
>+  size_t new_data_size;
>+  uint16_t *new_data;
>+
>+  /* Check whether this entry is already in the boot order.  If it is, leave
>+     it alone.  */
>+  data = (uint16_t *) order->data;
>+  for (i = 0; i < order->data_size / sizeof (uint16_t); ++i)
>+    if (data[i] == num)
>+      return;
>+
>+  new_data_size = order->data_size + sizeof (uint16_t);
>+  new_data = xmalloc (new_data_size);
>+  new_data[0] = num;
>+  memcpy (new_data + 1, order->data, order->data_size);
>+  free (order->data);
>+  order->data = (uint8_t *) new_data;
>+  order->data_size = new_data_size;
>+}
>+
>+static int
>+find_free_boot_num (struct efi_variable *entries)
>+{
>+  int num_vars = 0, i;
>+  struct efi_variable *entry;
>+
>+  FOR_LIST_ELEMENTS (entry, entries)
>+    ++num_vars;
>+
>+  if (num_vars == 0)
>+    return 0;
>+
>+  /* O(n^2), but n is small and this is easy. */

Nod, fair point. Never seen more than a dozen or so boot entries on a
real system.

>+  for (i = 0; i < num_vars; ++i)
>+    {
>+      int found = 0;
>+      FOR_LIST_ELEMENTS (entry, entries)
>+	{
>+	  if (entry->num == i)
>+	    {
>+	      found = 1;
>+	      break;
>+	    }
>+	}
>+      if (!found)
>+	return i;
>+    }
>+
>+  return i;
>+}
>+
>+static int
>+get_edd_version (void)
>+{
>+  efi_guid_t blkx_guid = BLKX_UNKNOWN_GUID;
>+  uint8_t *data = NULL;
>+  size_t data_size = 0;
>+  uint32_t attributes;
>+  efidp_header *path;
>+  int rc;
>+
>+  rc = efi_get_variable (blkx_guid, "blk0", &data, &data_size, &attributes);
>+  if (rc < 0)
>+    return rc;
>+
>+  path = (efidp_header *) data;
>+  if (path->type == 2 && path->subtype == 1)
>+    return 3;
>+  return 1;
>+}
>+
>+static struct efi_variable *
>+make_boot_variable (int num, const char *disk, int part, const char *loader,
>+		    const char *label)
>+{
>+  struct efi_variable *entry = new_boot_variable ();
>+  uint32_t options;
>+  uint32_t edd10_devicenum;
>+  ssize_t dp_needed, loadopt_needed;
>+  efidp dp = NULL;
>+
>+  options = EFIBOOT_ABBREV_HD;
>+  switch (get_edd_version ()) {
>+    case 1:
>+      options = EFIBOOT_ABBREV_EDD10;
>+      break;
>+    case 3:
>+      options = EFIBOOT_ABBREV_NONE;
>+      break;
>+  }
>+
>+  /* This may not be the right disk; but it's probably only an issue on very
>+     old hardware anyway. */
>+  edd10_devicenum = 0x80;
>+
>+  dp_needed = efi_generate_file_device_path_from_esp (NULL, 0, disk, part,
>+						      loader, options,
>+						      edd10_devicenum);
>+  if (dp_needed < 0)
>+    goto err;
>+
>+  dp = xmalloc (dp_needed);
>+  dp_needed = efi_generate_file_device_path_from_esp ((uint8_t *) dp,
>+						      dp_needed, disk, part,
>+						      loader, options,
>+						      edd10_devicenum);
>+  if (dp_needed < 0)
>+    goto err;
>+
>+  loadopt_needed = efi_loadopt_create (NULL, 0, LOAD_OPTION_ACTIVE,
>+				       dp, dp_needed, (unsigned char *) label,
>+				       NULL, 0);
>+  if (loadopt_needed < 0)
>+    goto err;
>+  entry->data_size = loadopt_needed;
>+  entry->data = xmalloc (entry->data_size);
>+  loadopt_needed = efi_loadopt_create (entry->data, entry->data_size,
>+				       LOAD_OPTION_ACTIVE, dp, dp_needed,
>+				       (unsigned char *) label, NULL, 0);
>+  if (loadopt_needed < 0)
>+    goto err;
>+
>+  entry->name = xasprintf ("Boot%04X", num);
>+  entry->num = num;
>+
>+  return entry;
>+
>+err:
>+  free_efi_variable (entry);
>+  free (dp);
>+  return NULL;
>+}
>+
>+int
>+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
>+				  const char *efifile_path,
>+				  const char *efi_distributor)
>+{
>+  const char *efidir_disk;
>+  int efidir_part;
>+  struct efi_variable *entries = NULL, *entry;
>+  struct efi_variable *order;
>+  int entry_num = -1;
>+  int rc;
>+
>+  efidir_disk = grub_util_biosdisk_get_osdev (efidir_grub_dev->disk);
>+  efidir_part = efidir_grub_dev->disk->partition ? efidir_grub_dev->disk->partition->number + 1 : 1;
>+
>+#ifdef __linux__
>+  /*
>+   * Linux uses efivarfs (mounted on /sys/firmware/efi/efivars) to access the
>+   * EFI variable store. Some legacy systems may still use the deprecated
>+   * efivars interface (accessed through /sys/firmware/efi/vars). Where both
>+   * are present, libefivar will use the former in preference, so attempting
>+   * to load efivars will not interfere with later operations.
>+   */
>+  grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", NULL },
>+			       NULL, NULL, "/dev/null");
>+#endif
>+
>+  if (!efi_variables_supported ())
>+    {
>+      grub_util_warn ("%s",
>+		      _("EFI variables are not supported on this system."));
>+      /* Let the user continue.  Perhaps they can still arrange to boot GRUB
>+         manually.  */
>+      return 0;
>+    }
>+
>+  rc = read_boot_variables (&entries);
>+  if (rc < 0)
>+    {
>+      grub_util_warn ("%s", _("Cannot read EFI Boot* variables"));
>+      goto err;
>+    }
>+  rc = read_efi_variable ("BootOrder", &order);
>+  if (rc < 0)
>+    {
>+      order = new_boot_variable ();
>+      order->name = xstrdup ("BootOrder");
>+      efi_error_clear ();
>+    }
>+
>+  /* Delete old entries from the same distributor.  */
>+  FOR_LIST_ELEMENTS (entry, entries)
>+    {
>+      efi_load_option *load_option = (efi_load_option *) entry->data;
>+      const char *label;
>+
>+      if (entry->num < 0)
>+	continue;
>+      label = (const char *) efi_loadopt_desc (load_option, entry->data_size);
>+      if (strcasecmp (label, efi_distributor) != 0)
>+	continue;
>+
>+      /* To avoid problems with some firmware implementations, reuse the first
>+         matching variable we find rather than deleting and recreating it.  */
>+      if (entry_num == -1)
>+	entry_num = entry->num;
>+      else
>+	{
>+	  grub_util_info ("deleting superfluous EFI variable %s (%s)",
>+			  entry->name, label);
>+	  rc = efi_del_variable (EFI_GLOBAL_GUID, entry->name);
>+	  if (rc < 0)
>+	    {
>+	      grub_util_warn (_("Cannot delete EFI variable %s"), entry->name);
>+	      goto err;
>+	    }
>+	}
>+
>+      remove_from_boot_order (order, (uint16_t) entry->num);
>+    }
>+
>+  if (entry_num == -1)
>+    entry_num = find_free_boot_num (entries);
>+  entry = make_boot_variable (entry_num, efidir_disk, efidir_part,
>+			      efifile_path, efi_distributor);
>+  if (!entry)
>+    goto err;
>+
>+  grub_util_info ("setting EFI variable %s", entry->name);
>+  rc = set_efi_variable (entry->name, entry);
>+  if (rc < 0)
>+    goto err;
>+
>+  add_to_boot_order (order, (uint16_t) entry_num);
>+
>+  grub_util_info ("setting EFI variable BootOrder");
>+  rc = set_efi_variable ("BootOrder", order);
>+  if (rc < 0)
>+    goto err;
>+
>+  return 0;
>+
>+err:
>+  show_efi_errors ();
>+  return errno;
>+}
>+
>+#endif /* HAVE_EFIVAR */
>diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
>index 55b8f4016..9d4530a38 100644
>--- a/grub-core/osdep/unix/platform.c
>+++ b/grub-core/osdep/unix/platform.c
>@@ -19,15 +19,12 @@
> #include <config.h>
> 
> #include <grub/util/install.h>
>-#include <grub/emu/hostdisk.h>
> #include <grub/util/misc.h>
> #include <grub/misc.h>
> #include <grub/i18n.h>
> #include <grub/emu/exec.h>
> #include <sys/types.h>
>-#include <dirent.h>
> #include <string.h>
>-#include <errno.h>
> 
> static char *
> get_ofpathname (const char *dev)
>@@ -78,102 +75,19 @@ get_ofpathname (const char *dev)
> 		   dev);
> }
> 
>-static int
>-grub_install_remove_efi_entries_by_distributor (const char *efi_distributor)
>-{
>-  int fd;
>-  pid_t pid = grub_util_exec_pipe ((const char * []){ "efibootmgr", NULL }, &fd);
>-  char *line = NULL;
>-  size_t len = 0;
>-  int rc = 0;
>-
>-  if (!pid)
>-    {
>-      grub_util_warn (_("Unable to open stream from %s: %s"),
>-		      "efibootmgr", strerror (errno));
>-      return errno;
>-    }
>-
>-  FILE *fp = fdopen (fd, "r");
>-  if (!fp)
>-    {
>-      grub_util_warn (_("Unable to open stream from %s: %s"),
>-		      "efibootmgr", strerror (errno));
>-      return errno;
>-    }
>-
>-  line = xmalloc (80);
>-  len = 80;
>-  while (1)
>-    {
>-      int ret;
>-      char *bootnum;
>-      ret = getline (&line, &len, fp);
>-      if (ret == -1)
>-	break;
>-      if (grub_memcmp (line, "Boot", sizeof ("Boot") - 1) != 0
>-	  || line[sizeof ("Boot") - 1] < '0'
>-	  || line[sizeof ("Boot") - 1] > '9')
>-	continue;
>-      if (!strcasestr (line, efi_distributor))
>-	continue;
>-      bootnum = line + sizeof ("Boot") - 1;
>-      bootnum[4] = '\0';
>-      if (!verbosity)
>-	rc = grub_util_exec ((const char * []){ "efibootmgr", "-q",
>-	      "-b", bootnum,  "-B", NULL });
>-      else
>-	rc = grub_util_exec ((const char * []){ "efibootmgr",
>-	      "-b", bootnum, "-B", NULL });
>-    }
>-
>-  free (line);
>-  return rc;
>-}
>-
> int
> grub_install_register_efi (grub_device_t efidir_grub_dev,
> 			   const char *efifile_path,
> 			   const char *efi_distributor)
> {
>-  const char * efidir_disk;
>-  int efidir_part;
>-  int ret;
>-  efidir_disk = grub_util_biosdisk_get_osdev (efidir_grub_dev->disk);
>-  efidir_part = efidir_grub_dev->disk->partition ? efidir_grub_dev->disk->partition->number + 1 : 1;
>-
>-  if (grub_util_exec_redirect_null ((const char * []){ "efibootmgr", "--version", NULL }))
>-    {
>-      /* TRANSLATORS: This message is shown when required executable `%s'
>-	 isn't found.  */
>-      grub_util_error (_("%s: not found"), "efibootmgr");
>-    }
>-
>-  /* On Linux, we need the efivars kernel modules.  */
>-#ifdef __linux__
>-  grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL });
>+#ifdef HAVE_EFIVAR
>+  return grub_install_efivar_register_efi (efidir_grub_dev, efifile_path,
>+					   efi_distributor);
>+#else
>+  grub_util_error ("%s",
>+		   _("GRUB was not built with efivar support; "
>+		     "cannot register EFI boot entry"));
> #endif
>-  /* Delete old entries from the same distributor.  */
>-  ret = grub_install_remove_efi_entries_by_distributor (efi_distributor);
>-  if (ret)
>-    return ret;
>-
>-  char *efidir_part_str = xasprintf ("%d", efidir_part);
>-
>-  if (!verbosity)
>-    ret = grub_util_exec ((const char * []){ "efibootmgr", "-q",
>-	  "-c", "-d", efidir_disk,
>-	  "-p", efidir_part_str, "-w",
>-	  "-L", efi_distributor, "-l", 
>-	  efifile_path, NULL });
>-  else
>-    ret = grub_util_exec ((const char * []){ "efibootmgr",
>-	  "-c", "-d", efidir_disk,
>-	  "-p", efidir_part_str, "-w",
>-	  "-L", efi_distributor, "-l", 
>-	  efifile_path, NULL });
>-  free (efidir_part_str);
>-  return ret;
> }
> 
> void
>diff --git a/include/grub/util/install.h b/include/grub/util/install.h
>index 2631b1074..dcad16ac5 100644
>--- a/include/grub/util/install.h
>+++ b/include/grub/util/install.h
>@@ -216,6 +216,11 @@ grub_install_get_default_arm_platform (void);
> const char *
> grub_install_get_default_x86_platform (void);
> 
>+int
>+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
>+				  const char *efifile_path,
>+				  const char *efi_distributor);
>+
> int
> grub_install_register_efi (grub_device_t efidir_grub_dev,
> 			   const char *efifile_path,
>diff --git a/util/grub-install.c b/util/grub-install.c
>index 264f9ecdc..a61df056e 100644
>--- a/util/grub-install.c
>+++ b/util/grub-install.c
>@@ -1885,7 +1885,7 @@ main (int argc, char *argv[])
> 					       "\\System\\Library\\CoreServices",
> 					       efi_distributor);
> 	      if (ret)
>-	        grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
>+	        grub_util_error (_("failed to register the EFI boot entry: %s"),
> 				 strerror (ret));
> 	    }
> 
>@@ -1929,7 +1929,7 @@ main (int argc, char *argv[])
> 	  ret = grub_install_register_efi (efidir_grub_dev,
> 					   efifile_path, efi_distributor);
> 	  if (ret)
>-	    grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
>+	    grub_util_error (_("failed to register the EFI boot entry: %s"),
> 			     strerror (ret));
> 	}
>       break;
>-- 
>2.17.1
>
-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
"This dress doesn't reverse." -- Alden Spiess

Re: [PATCH 2/2] Minimise writes to EFI variable storage

[Colin Watson]

On Wed, Mar 13, 2019 at 01:07:20AM +0000, Steve McIntyre wrote:
> On Mon, Mar 11, 2019 at 03:05:46PM +0000, Colin Watson wrote:
> >+/* Boot option attributes.  */
> >+#define LOAD_OPTION_ACTIVE 0x00000001
> >+
> >+/* GUIDs.  */
> >+#define BLKX_UNKNOWN_GUID \
> >+  EFI_GUID (0x47c7b225, 0xc42a, 0x11d2, 0x8e57, 0x00, 0xa0, 0xc9, 0x69, \
> >+	    0x72, 0x3b)
> 
> Ugh. I'm assuming the mahic numbers here are not exposed usefully by
> efivar or efiboot?

Sadly not, as far as I can see.

> So new_efi_variable() is using xmalloc() so it's safe if the
> allocation fails. But what happens if efi_get_variable() fails - do
> you need to free all the members by calling free_efi_variable() rather
> than simply free() here?

Quite right, and there was a similar bug in another place.  I'll send a
v2 fixing this and your comment about my grub_lltoa change.

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 2/2] Minimise writes to EFI variable storage

[Steve McIntyre]

On Fri, Mar 22, 2019 at 11:29:15PM +0000, Colin Watson wrote:
>On Wed, Mar 13, 2019 at 01:07:20AM +0000, Steve McIntyre wrote:
>> On Mon, Mar 11, 2019 at 03:05:46PM +0000, Colin Watson wrote:
>> >+/* Boot option attributes.  */
>> >+#define LOAD_OPTION_ACTIVE 0x00000001
>> >+
>> >+/* GUIDs.  */
>> >+#define BLKX_UNKNOWN_GUID \
>> >+  EFI_GUID (0x47c7b225, 0xc42a, 0x11d2, 0x8e57, 0x00, 0xa0, 0xc9, 0x69, \
>> >+	    0x72, 0x3b)
>> 
>> Ugh. I'm assuming the mahic numbers here are not exposed usefully by
>> efivar or efiboot?
>
>Sadly not, as far as I can see.

:-( That sounds like a clear bug, then. IMHO it's clearly part of the
interface that they should be providing.

>> So new_efi_variable() is using xmalloc() so it's safe if the
>> allocation fails. But what happens if efi_get_variable() fails - do
>> you need to free all the members by calling free_efi_variable() rather
>> than simply free() here?
>
>Quite right, and there was a similar bug in another place.  I'll send a
>v2 fixing this and your comment about my grub_lltoa change.

ACK, and then a v3. Will look again shortly...

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
Welcome my son, welcome to the machine.

Re: [PATCH 0/2] Minimise writes to EFI variable storage

[Colin Watson]

Oops, please ignore me failing to drive my email client correctly.
Corrected subject line here ...

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Daniel Kiper]

On Mon, Mar 11, 2019 at 03:04:49PM +0000, Colin Watson wrote:
> Some UEFI firmware is easily provoked into running out of space in its
> variable storage.  This is usually due to certain kernel drivers (e.g.
> pstore), but regardless of the cause it can cause grub-install to fail
> because it currently asks efibootmgr to delete and re-add entries, and
> the deletion often doesn't result in an immediate garbage collection.
> Writing variables frequently also increases wear on the NVRAM which may
> have limited write cycles.  For these reasons, it's desirable to find a
> way to minimise writes while still allowing grub-install to ensure that
> a suitable boot entry exists.
>
> This short patch series does so by using the efivar and efiboot
> libraries directly.

This looks like something for longer discussion. So, I am going to take
the patchset after the release if you do not convince me that it should
land in the GRUB 2.04.

And I agree with Steve comments.

Daniel

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Colin Watson]

On Wed, Mar 13, 2019 at 10:56:47AM +0100, Daniel Kiper wrote:
> This looks like something for longer discussion. So, I am going to take
> the patchset after the release if you do not convince me that it should
> land in the GRUB 2.04.

While I think it's important and I expect to be applying some variation
of it to Debian for our upcoming release, it's also not in any way a new
issue, so I think it's more important to get more timely and frequent
GRUB releases happening than to push for this particular patch set to be
in 2.04.

> And I agree with Steve comments.

Yep - I'll deal with those soon and post a v2.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Daniel Kiper]

On Wed, Mar 13, 2019 at 10:12:26AM +0000, Colin Watson wrote:
> On Wed, Mar 13, 2019 at 10:56:47AM +0100, Daniel Kiper wrote:
> > This looks like something for longer discussion. So, I am going to take
> > the patchset after the release if you do not convince me that it should
> > land in the GRUB 2.04.
>
> While I think it's important and I expect to be applying some variation
> of it to Debian for our upcoming release, it's also not in any way a new
> issue, so I think it's more important to get more timely and frequent
> GRUB releases happening than to push for this particular patch set to be
> in 2.04.

So, after the release. Granted!

> > And I agree with Steve comments.
>
> Yep - I'll deal with those soon and post a v2.

Thanks!

Daniel