From 81b5defaa6fe0f89acdd60f37009ee5e966a9bf0 Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Fri, 16 Oct 2020 18:38:02 +0100 Subject: [PATCH] uefi: Use fu_efivar_get_data() to fix setting BootNext correctly Fixes https://github.com/fwupd/fwupd/issues/2169 --- libfwupdplugin/fu-efivar.c | 47 ++++++++++++++++++++ libfwupdplugin/fu-efivar.h | 2 + libfwupdplugin/fu-self-test.c | 7 +++ libfwupdplugin/fwupdplugin.map | 8 +++- plugins/uefi/fu-uefi-bootmgr.c | 78 +++++++++++++++------------------- 5 files changed, 98 insertions(+), 44 deletions(-) diff --git a/libfwupdplugin/fu-efivar.c b/libfwupdplugin/fu-efivar.c index 4bcd15d9b..912b61fdc 100644 --- a/libfwupdplugin/fu-efivar.c +++ b/libfwupdplugin/fu-efivar.c @@ -322,6 +322,53 @@ fu_efivar_get_data (const gchar *guid, const gchar *name, guint8 **data, #endif } +/** + * fu_efivar_get_names: + * @guid: Globally unique identifier + * @error: A #GError + * + * Gets the list of names where the GUID matches. An error is set if there are + * no names matching the GUID. + * + * Returns: (transfer container) (element-type utf8): array of names + * + * Since: 1.4.7 + **/ +GPtrArray * +fu_efivar_get_names (const gchar *guid, GError **error) +{ + const gchar *name_guid; + g_autofree gchar *path = fu_efivar_get_path (); + g_autoptr(GDir) dir = NULL; + g_autoptr(GPtrArray) names = g_ptr_array_new_with_free_func (g_free); + + /* find names with matching GUID */ + dir = g_dir_open (path, 0, error); + if (dir == NULL) + return NULL; + while ((name_guid = g_dir_read_name (dir)) != NULL) { + gsize name_guidsz = strlen (name_guid); + if (name_guidsz < 38) + continue; + if (g_strcmp0 (name_guid + name_guidsz - 36, guid) == 0) { + g_ptr_array_add (names, + g_strndup (name_guid, name_guidsz - 37)); + } + } + + /* nothing found */ + if (names->len == 0) { + g_set_error (error, + G_IO_ERROR, + G_IO_ERROR_NOT_FOUND, + "no names for GUID %s", guid); + return NULL; + } + + /* success */ + return g_steal_pointer (&names); +} + /** * fu_efivar_set_data: * @guid: Globally unique identifier diff --git a/libfwupdplugin/fu-efivar.h b/libfwupdplugin/fu-efivar.h index 926e60aec..0401083de 100644 --- a/libfwupdplugin/fu-efivar.h +++ b/libfwupdplugin/fu-efivar.h @@ -43,5 +43,7 @@ gboolean fu_efivar_delete (const gchar *guid, gboolean fu_efivar_delete_with_glob (const gchar *guid, const gchar *name_glob, GError **error); +GPtrArray *fu_efivar_get_names (const gchar *guid, + GError **error); gboolean fu_efivar_secure_boot_enabled (void); gboolean fu_efivar_secure_boot_enabled_full(GError **error); diff --git a/libfwupdplugin/fu-self-test.c b/libfwupdplugin/fu-self-test.c index f218acb4b..2c10980b3 100644 --- a/libfwupdplugin/fu-self-test.c +++ b/libfwupdplugin/fu-self-test.c @@ -1808,6 +1808,7 @@ fu_efivar_func (void) guint32 attr = 0; g_autofree guint8 *data = NULL; g_autoptr(GError) error = NULL; + g_autoptr(GPtrArray) names = NULL; /* check supported */ ret = fu_efivar_supported (&error); @@ -1818,6 +1819,12 @@ fu_efivar_func (void) g_assert_false (fu_efivar_exists (FU_EFIVAR_GUID_EFI_GLOBAL, "NotGoingToExist")); g_assert_true (fu_efivar_exists (FU_EFIVAR_GUID_EFI_GLOBAL, "SecureBoot")); + /* list a few keys */ + names = fu_efivar_get_names (FU_EFIVAR_GUID_EFI_GLOBAL, &error); + g_assert_no_error (error); + g_assert_nonnull (names); + g_assert_cmpint (names->len, ==, 2); + /* write and read a key */ ret = fu_efivar_set_data (FU_EFIVAR_GUID_EFI_GLOBAL, "Test", (guint8 *) "1", 1, diff --git a/libfwupdplugin/fwupdplugin.map b/libfwupdplugin/fwupdplugin.map index aca29f593..b48b2ece0 100644 --- a/libfwupdplugin/fwupdplugin.map +++ b/libfwupdplugin/fwupdplugin.map @@ -610,6 +610,12 @@ LIBFWUPDPLUGIN_1.4.6 { local: *; } LIBFWUPDPLUGIN_1.4.5; +LIBFWUPDPLUGIN_1.4.7 { + global: + fu_efivar_get_names; + local: *; +} LIBFWUPDPLUGIN_1.4.6; + LIBFWUPDPLUGIN_1.5.0 { global: fu_common_cpuid; @@ -659,4 +665,4 @@ LIBFWUPDPLUGIN_1.5.0 { fu_udev_device_get_subsystem_model; fu_udev_device_get_subsystem_vendor; local: *; -} LIBFWUPDPLUGIN_1.4.6; +} LIBFWUPDPLUGIN_1.4.7; diff --git a/plugins/uefi/fu-uefi-bootmgr.c b/plugins/uefi/fu-uefi-bootmgr.c index 30dbb7a7a..8b04335f2 100644 --- a/plugins/uefi/fu-uefi-bootmgr.c +++ b/plugins/uefi/fu-uefi-bootmgr.c @@ -93,23 +93,26 @@ static gboolean fu_uefi_setup_bootnext_with_dp (const guint8 *dp_buf, guint8 *opt, gssize opt_size, GError **error) { const gchar *desc; - efi_guid_t *guid = NULL; + const gchar *name; efi_load_option *loadopt = NULL; - gchar *name = NULL; gint rc; gsize var_data_size = 0; guint32 attr; guint16 boot_next = G_MAXUINT16; g_autofree guint8 *var_data = NULL; g_autofree guint8 *set_entries = g_malloc0 (G_MAXUINT16); + g_autoptr(GPtrArray) names = NULL; - while ((rc = efi_get_next_variable_name (&guid, &name)) > 0) { + names = fu_efivar_get_names (FU_EFIVAR_GUID_EFI_GLOBAL, error); + if (names == NULL) + return FALSE; + for (guint i = 0; i < names->len; i++) { gint scanned = 0; guint16 entry = 0; g_autofree guint8 *var_data_tmp = NULL; + g_autoptr(GError) error_local = NULL; - if (efi_guid_cmp (guid, &efi_guid_global) != 0) - continue; + name = g_ptr_array_index (names, i); rc = sscanf (name, "Boot%hX%n", &entry, &scanned); if (rc < 0) { g_set_error (error, @@ -126,9 +129,11 @@ fu_uefi_setup_bootnext_with_dp (const guint8 *dp_buf, guint8 *opt, gssize opt_si /* mark this as used */ set_entries[entry] = 1; - rc = efi_get_variable (*guid, name, &var_data_tmp, &var_data_size, &attr); - if (rc < 0) { - g_debug ("efi_get_variable(%s) failed: %s", name, strerror(rc)); + if (!fu_efivar_get_data (FU_EFIVAR_GUID_EFI_GLOBAL, name, + &var_data_tmp, &var_data_size, + &attr, &error_local)) { + g_debug ("failed to get data for name %s: %s", + name, error_local->message); continue; } @@ -150,13 +155,6 @@ fu_uefi_setup_bootnext_with_dp (const guint8 *dp_buf, guint8 *opt, gssize opt_si efi_error_clear (); break; } - if (rc < 0) { - g_set_error_literal (error, - G_IO_ERROR, - G_IO_ERROR_FAILED, - "failed to find boot variable"); - return FALSE; - } /* already exists */ if (var_data != NULL) { @@ -165,12 +163,10 @@ fu_uefi_setup_bootnext_with_dp (const guint8 *dp_buf, guint8 *opt, gssize opt_si memcmp (var_data, opt, opt_size) != 0) { g_debug ("%s -> '%s' : updating existing boot entry", name, desc); efi_loadopt_attr_set (loadopt, LOAD_OPTION_ACTIVE); - rc = efi_set_variable (*guid, name, opt, opt_size, attr, 0644); - if (rc < 0) { - g_set_error_literal (error, - G_IO_ERROR, - G_IO_ERROR_FAILED, - "could not set boot variable active"); + if (!fu_efivar_set_data (FU_EFIVAR_GUID_EFI_GLOBAL, + name, opt, opt_size, attr, error)) { + g_prefix_error (error, + "could not set boot variable active: "); return FALSE; } } else { @@ -195,17 +191,15 @@ fu_uefi_setup_bootnext_with_dp (const guint8 *dp_buf, guint8 *opt, gssize opt_si } boot_next_name = g_strdup_printf ("Boot%04X", (guint) boot_next); g_debug ("%s -> creating new entry", boot_next_name); - rc = efi_set_variable (efi_guid_global, boot_next_name, opt, opt_size, - EFI_VARIABLE_NON_VOLATILE | - EFI_VARIABLE_BOOTSERVICE_ACCESS | - EFI_VARIABLE_RUNTIME_ACCESS, - 0644); - if (rc < 0) { - g_set_error (error, - G_IO_ERROR, - G_IO_ERROR_FAILED, - "could not set boot variable %s: %d", - boot_next_name, rc); + if (!fu_efivar_set_data (FU_EFIVAR_GUID_EFI_GLOBAL, + boot_next_name, opt, opt_size, + FU_EFIVAR_ATTR_NON_VOLATILE | + FU_EFIVAR_ATTR_BOOTSERVICE_ACCESS | + FU_EFIVAR_ATTR_RUNTIME_ACCESS, + error)) { + g_prefix_error (error, + "could not set boot variable %s: ", + boot_next_name); return FALSE; } } @@ -215,17 +209,15 @@ fu_uefi_setup_bootnext_with_dp (const guint8 *dp_buf, guint8 *opt, gssize opt_si return FALSE; /* set the boot next */ - rc = efi_set_variable (efi_guid_global, "BootNext", (guint8 *)&boot_next, 2, - EFI_VARIABLE_NON_VOLATILE | - EFI_VARIABLE_BOOTSERVICE_ACCESS | - EFI_VARIABLE_RUNTIME_ACCESS, - 0644); - if (rc < 0) { - g_set_error (error, - G_IO_ERROR, - G_IO_ERROR_FAILED, - "could not set BootNext(%" G_GUINT16_FORMAT ")", - boot_next); + if (!fu_efivar_set_data (FU_EFIVAR_GUID_EFI_GLOBAL, + "BootNext", (guint8 *)&boot_next, 2, + FU_EFIVAR_ATTR_NON_VOLATILE | + FU_EFIVAR_ATTR_BOOTSERVICE_ACCESS | + FU_EFIVAR_ATTR_RUNTIME_ACCESS, + error)) { + g_prefix_error (error, + "could not set BootNext(%" G_GUINT16_FORMAT "): ", + boot_next); return FALSE; } return TRUE;