On Sat, Mar 23, 2019 at 01:42:30PM +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. > > Fixes Debian bug #891434. > > Signed-off-by: Colin Watson <cjwat...@ubuntu.com> > --- > INSTALL | 5 + > Makefile.util.def | 20 ++ > configure.ac | 12 + > grub-core/osdep/efivar.c | 3 + > grub-core/osdep/unix/efivar.c | 508 ++++++++++++++++++++++++++++++++ > grub-core/osdep/unix/platform.c | 100 +------ > include/grub/util/install.h | 5 + > util/grub-install.c | 4 +- > 8 files changed, 562 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) > +
I do not think that we need separate section. Please add it to "On GNU/Linux, you also need" section below. > On GNU/Linux, you also need: > > * libdevmapper 1.02.34 or later (recommended) [...] > diff --git a/grub-core/osdep/unix/efivar.c b/grub-core/osdep/unix/efivar.c > new file mode 100644 > index 000000000..4a58328b4 > --- /dev/null > +++ b/grub-core/osdep/unix/efivar.c > @@ -0,0 +1,508 @@ > +/* > + * 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_dom...@dell.com> > + * Copyright 2015-2016 Red Hat, Inc. <pjo...@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)); Could you add empty line here? > + 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; And here? > + 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_efi_variable (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. */ Could you format the comment in the following way? /* * Latest Apple firmware sets the high bit which appears invalid * .... */ > + 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. */ Ditto. > +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); Is it possible to print an error message/code here? > + } > + 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; Please add empty line here. > + 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])); What this does? Could you add the comment here? > + } > + else if (newlist) > + { > + for (i = 0; i < nentries; ++i) > + free_efi_variable (newlist[i]); > + free (newlist); > + } > + return rc; > +} > + > +#define GET_ORDER(data, i) \ > + ((uint16_t) ((data)[(i) * 2]) + ((data)[(i) * 2 + 1] << 8)) > +#define SET_ORDER(data, i, num) \ > + do { \ > + (data)[(i) * 2] = (num) & 0xFF; \ > + (data)[(i) * 2 + 1] = ((num) >> 8) & 0xFF; \ > + } while (0) Could you define all stuff like that just before the first function in the file? > +static void > +remove_from_boot_order (struct efi_variable *order, uint16_t num) > +{ > + 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. */ Please fix the comment like I suggested earlier. > + for (old_i = 0, new_i = 0; > + old_i < order->data_size / sizeof (uint16_t); > + ++old_i) > + { > + uint16_t old_num = GET_ORDER (order->data, old_i); > + if (old_num != num) > + { > + if (new_i != old_i) > + SET_ORDER (order->data, new_i, old_num); > + ++new_i; > + } > + } > + > + order->data_size = new_i * sizeof (uint16_t); > +} > + > +static void > +add_to_boot_order (struct efi_variable *order, uint16_t num) > +{ > + int i; > + size_t new_data_size; > + uint8_t *new_data; > + > + /* Check whether this entry is already in the boot order. If it is, leave > + it alone. */ Ditto and below please... > + for (i = 0; i < order->data_size / sizeof (uint16_t); ++i) > + if (GET_ORDER (order->data, i) == num) > + return; > + > + new_data_size = order->data_size + sizeof (uint16_t); > + new_data = xmalloc (new_data_size); xrealloc()? > + SET_ORDER (new_data, 0, num); > + memcpy (new_data + sizeof (uint16_t), order->data, order->data_size); > + free (order->data); > + order->data = 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; > + } Could you explain in the comment how this function works? > + 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; Could not you (re)use constants instead of plain numbers here? > +} > + > +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: Ditto. > + 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")); Could you add error messaged/code here? > + 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); Ditto and below please... Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
