From 5ee3c914a4e904567e66654177b07777dde0d100 Mon Sep 17 00:00:00 2001 From: Emil Velikov Date: Wed, 4 Oct 2023 11:51:47 +0100 Subject: [PATCH] sd-boot: introduce and use efivar_unset() Currently some of the code base check for the variable presence before removing it, and some do not. More so, in all cases (being updated) we're dealing with non-volatile variables where changing those attribute to NVRAM wear out. From what information I could find, there is no definitive answer if the UEFI implementation will write to the NVRAM even when the variable is missing. So add a simple helper that checks for the variable presence before removing it. While also having a bit cleaner API than the current efivar_set(..., NULL, ...); efivar_unset() follows the design from efivar_set*() where it returns an EFI_STATUS even though its (presently) unused. v2: - add inline comment, use early return v3: - typos? typos! Signed-off-by: Emil Velikov --- src/boot/efi/boot.c | 10 +++++----- src/boot/efi/util.c | 15 +++++++++++++++ src/boot/efi/util.h | 2 ++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 6dad2bc3c4..7b13cd21db 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1058,7 +1058,7 @@ static bool menu_run( if (console_mode_efivar_saved != config->console_mode_efivar) { if (config->console_mode_efivar == CONSOLE_MODE_KEEP) - efivar_set(MAKE_GUID_PTR(LOADER), u"LoaderConfigConsoleMode", NULL, EFI_VARIABLE_NON_VOLATILE); + efivar_unset(MAKE_GUID_PTR(LOADER), u"LoaderConfigConsoleMode", EFI_VARIABLE_NON_VOLATILE); else efivar_set_uint_string(MAKE_GUID_PTR(LOADER), u"LoaderConfigConsoleMode", config->console_mode_efivar, EFI_VARIABLE_NON_VOLATILE); @@ -1067,7 +1067,7 @@ static bool menu_run( if (timeout_efivar_saved != config->timeout_sec_efivar) { switch (config->timeout_sec_efivar) { case TIMEOUT_UNSET: - efivar_set(MAKE_GUID_PTR(LOADER), u"LoaderConfigTimeout", NULL, EFI_VARIABLE_NON_VOLATILE); + efivar_unset(MAKE_GUID_PTR(LOADER), u"LoaderConfigTimeout", EFI_VARIABLE_NON_VOLATILE); break; case TIMEOUT_MENU_FORCE: efivar_set(MAKE_GUID_PTR(LOADER), u"LoaderConfigTimeout", u"menu-force", EFI_VARIABLE_NON_VOLATILE); @@ -1618,7 +1618,7 @@ static void config_load_defaults(Config *config, EFI_FILE *root_dir) { err = efivar_get_timeout(u"LoaderConfigTimeoutOneShot", &config->timeout_sec); if (err == EFI_SUCCESS) { /* Unset variable now, after all it's "one shot". */ - (void) efivar_set(MAKE_GUID_PTR(LOADER), u"LoaderConfigTimeoutOneShot", NULL, EFI_VARIABLE_NON_VOLATILE); + (void) efivar_unset(MAKE_GUID_PTR(LOADER), u"LoaderConfigTimeoutOneShot", EFI_VARIABLE_NON_VOLATILE); config->force_menu = true; /* force the menu when this is set */ } else if (err != EFI_NOT_FOUND) @@ -1631,7 +1631,7 @@ static void config_load_defaults(Config *config, EFI_FILE *root_dir) { err = efivar_get(MAKE_GUID_PTR(LOADER), u"LoaderEntryOneShot", &config->entry_oneshot); if (err == EFI_SUCCESS) /* Unset variable now, after all it's "one shot". */ - (void) efivar_set(MAKE_GUID_PTR(LOADER), u"LoaderEntryOneShot", NULL, EFI_VARIABLE_NON_VOLATILE); + (void) efivar_unset(MAKE_GUID_PTR(LOADER), u"LoaderEntryOneShot", EFI_VARIABLE_NON_VOLATILE); (void) efivar_get(MAKE_GUID_PTR(LOADER), u"LoaderEntryDefault", &config->entry_default_efivar); @@ -2503,7 +2503,7 @@ static void save_selected_entry(const Config *config, const ConfigEntry *entry) (void) efivar_set(MAKE_GUID_PTR(LOADER), u"LoaderEntryLastBooted", entry->id, EFI_VARIABLE_NON_VOLATILE); } else /* Delete the non-volatile var if not needed. */ - (void) efivar_set(MAKE_GUID_PTR(LOADER), u"LoaderEntryLastBooted", NULL, EFI_VARIABLE_NON_VOLATILE); + (void) efivar_unset(MAKE_GUID_PTR(LOADER), u"LoaderEntryLastBooted", EFI_VARIABLE_NON_VOLATILE); } static EFI_STATUS secure_boot_discover_keys(Config *config, EFI_FILE *root_dir) { diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index 60991d4718..bb3ccb8280 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -85,6 +85,21 @@ EFI_STATUS efivar_set_uint64_le(const EFI_GUID *vendor, const char16_t *name, ui return efivar_set_raw(vendor, name, buf, sizeof(buf), flags); } +EFI_STATUS efivar_unset(const EFI_GUID *vendor, const char16_t *name, uint32_t flags) { + EFI_STATUS err; + + assert(vendor); + assert(name); + + /* We could be wiping a non-volatile variable here and the spec makes no guarantees that won't incur + * in an extra write (and thus wear out). So check and clear only if needed. */ + err = efivar_get_raw(vendor, name, NULL, NULL); + if (err == EFI_SUCCESS) + return efivar_set_raw(vendor, name, NULL, 0, flags); + + return err; +} + EFI_STATUS efivar_get(const EFI_GUID *vendor, const char16_t *name, char16_t **ret) { _cleanup_free_ char16_t *buf = NULL; EFI_STATUS err; diff --git a/src/boot/efi/util.h b/src/boot/efi/util.h index 10620dabca..2f8c96d0ac 100644 --- a/src/boot/efi/util.h +++ b/src/boot/efi/util.h @@ -95,6 +95,8 @@ EFI_STATUS efivar_set_uint32_le(const EFI_GUID *vendor, const char16_t *NAME, ui EFI_STATUS efivar_set_uint64_le(const EFI_GUID *vendor, const char16_t *name, uint64_t value, uint32_t flags); void efivar_set_time_usec(const EFI_GUID *vendor, const char16_t *name, uint64_t usec); +EFI_STATUS efivar_unset(const EFI_GUID *vendor, const char16_t *name, uint32_t flags); + EFI_STATUS efivar_get(const EFI_GUID *vendor, const char16_t *name, char16_t **ret); EFI_STATUS efivar_get_raw(const EFI_GUID *vendor, const char16_t *name, char **ret, size_t *ret_size); EFI_STATUS efivar_get_uint_string(const EFI_GUID *vendor, const char16_t *name, size_t *ret); -- 2.25.1