From f862e847246b5588d9d6ed7d91da11b6adbf39e7 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 01/10] macro: Move some macros to macro-fundamental.h Also, make sure STRLEN works with wide strings too. --- src/basic/macro.h | 23 ----------------------- src/fundamental/macro-fundamental.h | 23 +++++++++++++++++++++++ src/test/test-format-util.c | 2 ++ 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/src/basic/macro.h b/src/basic/macro.h index 92498b0f20..3e89ba5e4d 100644 --- a/src/basic/macro.h +++ b/src/basic/macro.h @@ -23,28 +23,12 @@ #define _packed_ __attribute__((__packed__)) #define _malloc_ __attribute__((__malloc__)) #define _weak_ __attribute__((__weak__)) -#define _likely_(x) (__builtin_expect(!!(x), 1)) -#define _unlikely_(x) (__builtin_expect(!!(x), 0)) #define _public_ __attribute__((__visibility__("default"))) #define _hidden_ __attribute__((__visibility__("hidden"))) #define _weakref_(x) __attribute__((__weakref__(#x))) #define _alignas_(x) __attribute__((__aligned__(__alignof(x)))) #define _alignptr_ __attribute__((__aligned__(sizeof(void*)))) #define _warn_unused_result_ __attribute__((__warn_unused_result__)) -#if __GNUC__ >= 7 -#define _fallthrough_ __attribute__((__fallthrough__)) -#else -#define _fallthrough_ -#endif -/* Define C11 noreturn without and even on older gcc - * compiler versions */ -#ifndef _noreturn_ -#if __STDC_VERSION__ >= 201112L -#define _noreturn_ _Noreturn -#else -#define _noreturn_ __attribute__((__noreturn__)) -#endif -#endif #if !defined(HAS_FEATURE_MEMORY_SANITIZER) # if defined(__has_feature) @@ -209,13 +193,6 @@ static inline size_t GREEDY_ALLOC_ROUND_UP(size_t l) { return m; } -/* - * STRLEN - return the length of a string literal, minus the trailing NUL byte. - * Contrary to strlen(), this is a constant expression. - * @x: a string literal. - */ -#define STRLEN(x) (sizeof(""x"") - 1U) - /* * container_of - cast a member of a structure out to the containing structure * @ptr: the pointer to the member. diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index e9fb4d3938..91b24a1826 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -14,6 +14,22 @@ #define _used_ __attribute__((__used__)) #define _unused_ __attribute__((__unused__)) #define _cleanup_(x) __attribute__((__cleanup__(x))) +#define _likely_(x) (__builtin_expect(!!(x), 1)) +#define _unlikely_(x) (__builtin_expect(!!(x), 0)) +#if __GNUC__ >= 7 +#define _fallthrough_ __attribute__((__fallthrough__)) +#else +#define _fallthrough_ +#endif +/* Define C11 noreturn without and even on older gcc + * compiler versions */ +#ifndef _noreturn_ +#if __STDC_VERSION__ >= 201112L +#define _noreturn_ _Noreturn +#else +#define _noreturn_ __attribute__((__noreturn__)) +#endif +#endif #define XSTRINGIFY(x) #x #define STRINGIFY(x) XSTRINGIFY(x) @@ -233,3 +249,10 @@ (ptr) = NULL; \ _ptr_; \ }) + +/* + * STRLEN - return the length of a string literal, minus the trailing NUL byte. + * Contrary to strlen(), this is a constant expression. + * @x: a string literal. + */ +#define STRLEN(x) (sizeof(""x"") - sizeof(typeof(x[0]))) diff --git a/src/test/test-format-util.c b/src/test/test-format-util.c index 4e6c90adfa..306e0788d0 100644 --- a/src/test/test-format-util.c +++ b/src/test/test-format-util.c @@ -7,6 +7,8 @@ /* Do some basic checks on STRLEN() and DECIMAL_STR_MAX() */ assert_cc(STRLEN("xxx") == 3); assert_cc(STRLEN("") == 0); +assert_cc(STRLEN(L"xxx") == 3 * sizeof(wchar_t)); +assert_cc(STRLEN(L"") == 0); assert_cc(DECIMAL_STR_MAX(uint8_t) == 5); assert_cc(DECIMAL_STR_MAX(int8_t) == 5); assert_cc(DECIMAL_STR_MAX(uint64_t) == 22); From 8aba0eec499b762657f528988c2f093ac490620d Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 02/10] sd-boot: Unify error handling log_error_stall() and log_error_status_stall() will ensure the user has a chance to catch an error message by stalling and also forcing a lightred/black color on it. Also, convert several Print() calls to it since they are actually error messages. --- src/boot/efi/boot.c | 62 ++++++++++---------------------- src/boot/efi/random-seed.c | 73 +++++++++++++------------------------- src/boot/efi/stub.c | 24 ++++--------- src/boot/efi/util.c | 17 +++++++-- src/boot/efi/util.h | 9 +++++ 5 files changed, 75 insertions(+), 110 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 3249171ec1..d0a3bce159 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -527,7 +527,7 @@ static BOOLEAN menu_run( err = console_set_mode(&config->console_mode, config->console_mode_change); if (EFI_ERROR(err)) { uefi_call_wrapper(ST->ConOut->ClearScreen, 1, ST->ConOut); - Print(L"Error switching console mode to %ld: %r.\r", (UINT64)config->console_mode, err); + log_error_stall(L"Error switching console mode to %lu: %r", (UINT64)config->console_mode, err); } } else uefi_call_wrapper(ST->ConOut->ClearScreen, 1, ST->ConOut); @@ -1221,8 +1221,7 @@ static VOID config_entry_bump_counters( break; if (r != EFI_BUFFER_TOO_SMALL || file_info_size * 2 < file_info_size) { - Print(L"\nFailed to get file info for '%s': %r\n", old_path, r); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); + log_error_stall(L"Failed to get file info for '%s': %r", old_path, r); return; } @@ -1234,8 +1233,7 @@ static VOID config_entry_bump_counters( StrCpy(file_info->FileName, entry->next_name); r = uefi_call_wrapper(handle->SetInfo, 4, handle, &EfiFileInfoGuid, file_info_size, file_info); if (EFI_ERROR(r)) { - Print(L"\nFailed to rename '%s' to '%s', ignoring: %r\n", old_path, entry->next_name, r); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); + log_error_stall(L"Failed to rename '%s' to '%s', ignoring: %r", old_path, entry->next_name, r); return; } @@ -2165,18 +2163,12 @@ static EFI_STATUS image_start( EFI_STATUS err; path = FileDevicePath(entry->device, entry->loader); - if (!path) { - Print(L"Error getting device path."); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); - return EFI_INVALID_PARAMETER; - } + if (!path) + return log_error_status_stall(EFI_INVALID_PARAMETER, L"Error getting device path."); err = uefi_call_wrapper(BS->LoadImage, 6, FALSE, parent_image, path, NULL, 0, &image); - if (EFI_ERROR(err)) { - Print(L"Error loading %s: %r", entry->loader, err); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); - return err; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Error loading %s: %r", entry->loader, err); if (config->options_edit) options = config->options_edit; @@ -2190,8 +2182,7 @@ static EFI_STATUS image_start( err = uefi_call_wrapper(BS->OpenProtocol, 6, image, &LoadedImageProtocol, (VOID **)&loaded_image, parent_image, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL); if (EFI_ERROR(err)) { - Print(L"Error getting LoadedImageProtocol handle: %r", err); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); + log_error_stall(L"Error getting LoadedImageProtocol handle: %r", err); goto out_unload; } loaded_image->LoadOptions = options; @@ -2202,10 +2193,8 @@ static EFI_STATUS image_start( err = tpm_log_event(SD_TPM_PCR, (EFI_PHYSICAL_ADDRESS) (UINTN) loaded_image->LoadOptions, loaded_image->LoadOptionsSize, loaded_image->LoadOptions); - if (EFI_ERROR(err)) { - Print(L"Unable to add image options measurement: %r", err); - uefi_call_wrapper(BS->Stall, 1, 200 * 1000); - } + if (EFI_ERROR(err)) + log_error_stall(L"Unable to add image options measurement: %r", err); #endif } @@ -2231,9 +2220,7 @@ static EFI_STATUS reboot_into_firmware(VOID) { return err; err = uefi_call_wrapper(RT->ResetSystem, 4, EfiResetCold, EFI_SUCCESS, 0, NULL); - Print(L"Error calling ResetSystem: %r", err); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); - return err; + return log_error_status_stall(err, L"Error calling ResetSystem: %r", err); } static VOID config_free(Config *config) { @@ -2305,30 +2292,21 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) { err = uefi_call_wrapper(BS->OpenProtocol, 6, image, &LoadedImageProtocol, (VOID **)&loaded_image, image, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL); - if (EFI_ERROR(err)) { - Print(L"Error getting a LoadedImageProtocol handle: %r", err); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); - return err; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Error getting a LoadedImageProtocol handle: %r", err); /* export the device path this image is started from */ if (disk_get_part_uuid(loaded_image->DeviceHandle, uuid) == EFI_SUCCESS) efivar_set(LOADER_GUID, L"LoaderDevicePartUUID", uuid, 0); root_dir = LibOpenRoot(loaded_image->DeviceHandle); - if (!root_dir) { - Print(L"Unable to open root directory."); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); - return EFI_LOAD_ERROR; - } + if (!root_dir) + return log_error_status_stall(EFI_LOAD_ERROR, L"Unable to open root directory.", EFI_LOAD_ERROR); if (secure_boot_enabled() && shim_loaded()) { err = security_policy_install(); - if (EFI_ERROR(err)) { - Print(L"Error installing security policy: %r ", err); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); - return err; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Error installing security policy: %r", err); } /* the filesystem path to this image, to prevent adding ourselves to the menu */ @@ -2367,8 +2345,7 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) { } if (config.entry_count == 0) { - Print(L"No loader found. Configuration files in \\loader\\entries\\*.conf are needed."); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); + log_error_stall(L"No loader found. Configuration files in \\loader\\entries\\*.conf are needed."); goto out; } @@ -2440,8 +2417,7 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) { err = image_start(image, &config, entry); if (EFI_ERROR(err)) { graphics_mode(FALSE); - Print(L"\nFailed to execute %s (%s): %r\n", entry->title, entry->loader, err); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); + log_error_stall(L"Failed to execute %s (%s): %r", entry->title, entry->loader, err); goto out; } diff --git a/src/boot/efi/random-seed.c b/src/boot/efi/random-seed.c index 3e179851b0..939daf3e41 100644 --- a/src/boot/efi/random-seed.c +++ b/src/boot/efi/random-seed.c @@ -35,10 +35,8 @@ static EFI_STATUS acquire_rng(UINTN size, VOID **ret) { return log_oom(); err = uefi_call_wrapper(rng->GetRNG, 3, rng, NULL, size, data); - if (EFI_ERROR(err)) { - Print(L"Failed to acquire RNG data: %r\n", err); - return err; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Failed to acquire RNG data: %r", err); *ret = TAKE_PTR(data); return EFI_SUCCESS; @@ -149,14 +147,12 @@ static EFI_STATUS acquire_system_token(VOID **ret, UINTN *ret_size) { err = efivar_get_raw(LOADER_GUID, L"LoaderSystemToken", &data, &size); if (EFI_ERROR(err)) { if (err != EFI_NOT_FOUND) - Print(L"Failed to read LoaderSystemToken EFI variable: %r", err); + log_error_stall(L"Failed to read LoaderSystemToken EFI variable: %r", err); return err; } - if (size <= 0) { - Print(L"System token too short, ignoring."); - return EFI_NOT_FOUND; - } + if (size <= 0) + return log_error_status_stall(EFI_NOT_FOUND, L"System token too short, ignoring."); *ret = TAKE_PTR(data); *ret_size = size; @@ -209,8 +205,7 @@ static VOID validate_sha256(void) { sha256_finish_ctx(&hash, result); if (CompareMem(result, array[i].hash, HASH_VALUE_SIZE) != 0) { - Print(L"SHA256 failed validation.\n"); - uefi_call_wrapper(BS->Stall, 1, 120 * 1000 * 1000); + log_error_stall(L"SHA256 failed validation."); return; } } @@ -246,7 +241,7 @@ EFI_STATUS process_random_seed(EFI_FILE *root_dir, RandomSeedMode mode) { err = uefi_call_wrapper(root_dir->Open, 5, root_dir, &handle, (CHAR16*) L"\\loader\\random-seed", EFI_FILE_MODE_READ|EFI_FILE_MODE_WRITE, 0ULL); if (EFI_ERROR(err)) { if (err != EFI_NOT_FOUND && err != EFI_WRITE_PROTECTED) - Print(L"Failed to open random seed file: %r\n", err); + log_error_stall(L"Failed to open random seed file: %r", err); return err; } @@ -255,15 +250,11 @@ EFI_STATUS process_random_seed(EFI_FILE *root_dir, RandomSeedMode mode) { return log_oom(); size = info->FileSize; - if (size < RANDOM_MAX_SIZE_MIN) { - Print(L"Random seed file is too short?\n"); - return EFI_INVALID_PARAMETER; - } + if (size < RANDOM_MAX_SIZE_MIN) + return log_error_status_stall(EFI_INVALID_PARAMETER, L"Random seed file is too short."); - if (size > RANDOM_MAX_SIZE_MAX) { - Print(L"Random seed file is too large?\n"); - return EFI_INVALID_PARAMETER; - } + if (size > RANDOM_MAX_SIZE_MAX) + return log_error_status_stall(EFI_INVALID_PARAMETER, L"Random seed file is too large."); seed = AllocatePool(size); if (!seed) @@ -271,20 +262,14 @@ EFI_STATUS process_random_seed(EFI_FILE *root_dir, RandomSeedMode mode) { rsize = size; err = uefi_call_wrapper(handle->Read, 3, handle, &rsize, seed); - if (EFI_ERROR(err)) { - Print(L"Failed to read random seed file: %r\n", err); - return err; - } - if (rsize != size) { - Print(L"Short read on random seed file\n"); - return EFI_PROTOCOL_ERROR; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Failed to read random seed file: %r", err); + if (rsize != size) + return log_error_status_stall(EFI_PROTOCOL_ERROR, L"Short read on random seed file."); err = uefi_call_wrapper(handle->SetPosition, 2, handle, 0); - if (EFI_ERROR(err)) { - Print(L"Failed to seek to beginning of random seed file: %r\n", err); - return err; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Failed to seek to beginning of random seed file: %r", err); /* Request some random data from the UEFI RNG. We don't need this to work safely, but it's a good * idea to use it because it helps us for cases where users mistakenly include a random seed in @@ -299,27 +284,19 @@ EFI_STATUS process_random_seed(EFI_FILE *root_dir, RandomSeedMode mode) { /* Update the random seed on disk before we use it */ wsize = size; err = uefi_call_wrapper(handle->Write, 3, handle, &wsize, new_seed); - if (EFI_ERROR(err)) { - Print(L"Failed to write random seed file: %r\n", err); - return err; - } - if (wsize != size) { - Print(L"Short write on random seed file\n"); - return EFI_PROTOCOL_ERROR; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Failed to write random seed file: %r", err); + if (wsize != size) + return log_error_status_stall(EFI_PROTOCOL_ERROR, L"Short write on random seed file."); err = uefi_call_wrapper(handle->Flush, 1, handle); - if (EFI_ERROR(err)) { - Print(L"Failed to flush random seed file: %r\n"); - return err; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Failed to flush random seed file: %r", err); /* We are good to go */ err = efivar_set_raw(LOADER_GUID, L"LoaderRandomSeed", for_kernel, size, 0); - if (EFI_ERROR(err)) { - Print(L"Failed to write random seed to EFI variable: %r\n", err); - return err; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Failed to write random seed to EFI variable: %r", err); return EFI_SUCCESS; } diff --git a/src/boot/efi/stub.c b/src/boot/efi/stub.c index 082fe91c9e..82da1d3ec4 100644 --- a/src/boot/efi/stub.c +++ b/src/boot/efi/stub.c @@ -36,18 +36,12 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) { err = uefi_call_wrapper(BS->OpenProtocol, 6, image, &LoadedImageProtocol, (VOID **)&loaded_image, image, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL); - if (EFI_ERROR(err)) { - Print(L"Error getting a LoadedImageProtocol handle: %r ", err); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); - return err; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Error getting a LoadedImageProtocol handle: %r", err); err = pe_memory_locate_sections(loaded_image->ImageBase, sections, addrs, offs, szs); - if (EFI_ERROR(err)) { - Print(L"Unable to locate embedded .linux section: %r ", err); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); - return err; - } + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Unable to locate embedded .linux section: %r", err); if (szs[0] > 0) cmdline = (CHAR8 *)(loaded_image->ImageBase) + addrs[0]; @@ -72,10 +66,8 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) { err = tpm_log_event(SD_TPM_PCR, (EFI_PHYSICAL_ADDRESS) (UINTN) loaded_image->LoadOptions, loaded_image->LoadOptionsSize, loaded_image->LoadOptions); - if (EFI_ERROR(err)) { - Print(L"Unable to add image options measurement: %r", err); - uefi_call_wrapper(BS->Stall, 1, 200 * 1000); - } + if (EFI_ERROR(err)) + log_error_stall(L"Unable to add image options measurement: %r", err); #endif } @@ -126,7 +118,5 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) { (UINTN)loaded_image->ImageBase + addrs[2], szs[2]); graphics_mode(FALSE); - Print(L"Execution of embedded linux image failed: %r\n", err); - uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); - return err; + return log_error_status_stall(err, L"Execution of embedded linux image failed: %r", err); } diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index 0ca4d17b37..c6b78154ce 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -425,8 +425,21 @@ EFI_STATUS file_read(EFI_FILE_HANDLE dir, const CHAR16 *name, UINTN off, UINTN s return err; } +VOID log_error_stall(const CHAR16 *fmt, ...) { + va_list args; + + uefi_call_wrapper(ST->ConOut->SetAttribute, 2, ST->ConOut, EFI_LIGHTRED|EFI_BACKGROUND_BLACK); + + Print(L"\n"); + va_start(args, fmt); + VPrint(fmt, args); + va_end(args); + Print(L"\n"); + + uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); +} + EFI_STATUS log_oom(void) { - Print(L"Out of memory."); - (void) uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000); + log_error_stall(L"Out of memory."); return EFI_OUT_OF_RESOURCES; } diff --git a/src/boot/efi/util.h b/src/boot/efi/util.h index 1a42b01033..d3bf848a95 100644 --- a/src/boot/efi/util.h +++ b/src/boot/efi/util.h @@ -74,4 +74,13 @@ static inline void FileHandleClosep(EFI_FILE_HANDLE *handle) { #define UINT64_MAX ((UINT64) -1) #endif +VOID log_error_stall(const CHAR16 *fmt, ...); EFI_STATUS log_oom(void); + +/* This works just like log_error_errno() from userspace, but requires you + * to provide err a second time if you want to use %r in the message! */ +#define log_error_status_stall(err, fmt, ...) \ + ({ \ + log_error_stall(fmt, ##__VA_ARGS__); \ + err; \ + }) From b2cf9922a4cdf8a649c1cc91ba6a3436987d26fa Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 03/10] sd-boot: Don't use magic integer constants --- src/boot/efi/util.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index c6b78154ce..b048df0764 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -101,7 +101,7 @@ EFI_STATUS efivar_set(const EFI_GUID *vendor, const CHAR16 *name, const CHAR16 * EFI_STATUS efivar_set_uint_string(const EFI_GUID *vendor, const CHAR16 *name, UINTN i, UINT32 flags) { CHAR16 str[32]; - SPrint(str, 32, L"%u", i); + SPrint(str, ELEMENTSOF(str), L"%u", i); return efivar_set(vendor, name, str, flags); } @@ -132,35 +132,35 @@ EFI_STATUS efivar_set_uint64_le(const EFI_GUID *vendor, const CHAR16 *name, UINT } EFI_STATUS efivar_get(const EFI_GUID *vendor, const CHAR16 *name, CHAR16 **value) { - _cleanup_freepool_ CHAR8 *buf = NULL; + _cleanup_freepool_ CHAR16 *buf = NULL; EFI_STATUS err; CHAR16 *val; UINTN size; - err = efivar_get_raw(vendor, name, &buf, &size); + err = efivar_get_raw(vendor, name, (CHAR8**)&buf, &size); if (EFI_ERROR(err)) return err; /* Make sure there are no incomplete characters in the buffer */ - if ((size % 2) != 0) + if ((size % sizeof(CHAR16)) != 0) return EFI_INVALID_PARAMETER; if (!value) return EFI_SUCCESS; /* Return buffer directly if it happens to be NUL terminated already */ - if (size >= 2 && buf[size-2] == 0 && buf[size-1] == 0) { - *value = (CHAR16*) TAKE_PTR(buf); + if (size >= sizeof(CHAR16) && buf[size/sizeof(CHAR16)] == 0) { + *value = TAKE_PTR(buf); return EFI_SUCCESS; } /* Make sure a terminating NUL is available at the end */ - val = AllocatePool(size + 2); + val = AllocatePool(size + sizeof(CHAR16)); if (!val) return EFI_OUT_OF_RESOURCES; CopyMem(val, buf, size); - val[size/2] = 0; /* NUL terminate */ + val[size / sizeof(CHAR16)] = 0; /* NUL terminate */ *value = val; return EFI_SUCCESS; @@ -255,7 +255,7 @@ VOID efivar_set_time_usec(const EFI_GUID *vendor, const CHAR16 *name, UINT64 use if (usec == 0) return; - SPrint(str, 32, L"%ld", usec); + SPrint(str, ELEMENTSOF(str), L"%ld", usec); efivar_set(vendor, name, str, 0); } From b52fafb26d90b77cfc259fcbdab3c95a571bacb1 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 04/10] sd-boot: Fix possible null pointer dereference Auto entries are showing garbage for the version in print_status() because StrDuplicate does not expect null pointers. --- src/boot/efi/boot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index d0a3bce159..54d704f0d1 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1722,7 +1722,7 @@ static ConfigEntry *config_entry_add_loader( *entry = (ConfigEntry) { .type = type, .title = StrDuplicate(title), - .version = StrDuplicate(version), + .version = version ? StrDuplicate(version) : NULL, .device = device, .loader = StrDuplicate(loader), .id = StrDuplicate(id), From 7a7267bf55cac0a90f6a8a83841c56fdebb1e69c Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 05/10] sd-boot: Add assert implementation There is a ASSERT() macro from gnu-efi, but that does not show any output to ConOut. Having to do some additional setup just to get some debug output is tedious and outright difficult on real hardware. --- src/boot/efi/assert.c | 15 +++++++++++++++ src/boot/efi/meson.build | 9 +++++++-- src/fundamental/macro-fundamental.h | 9 ++++++++- 3 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 src/boot/efi/assert.c diff --git a/src/boot/efi/assert.c b/src/boot/efi/assert.c new file mode 100644 index 0000000000..350b2b0167 --- /dev/null +++ b/src/boot/efi/assert.c @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#ifndef NDEBUG + +#include +#include +#include "util.h" + +void efi_assert(const char *expr, const char *file, unsigned line, const char *function) { + log_error_stall(L"systemd-boot assertion '%a' failed at %a:%u, function %a(). Halting.", expr, file, line, function); + for (;;) + uefi_call_wrapper(BS->Stall, 1, 60 * 1000 * 1000); +} + +#endif diff --git a/src/boot/efi/meson.build b/src/boot/efi/meson.build index 0a96a72d6e..c96a16426a 100644 --- a/src/boot/efi/meson.build +++ b/src/boot/efi/meson.build @@ -17,6 +17,7 @@ efi_headers = files(''' '''.split()) common_sources = ''' + assert.c disk.c graphics.c measure.c @@ -219,12 +220,16 @@ if have_gnu_efi compile_args += ['-Werror'] endif if get_option('buildtype') == 'debug' - compile_args += ['-ggdb', '-O0'] + compile_args += ['-ggdb', '-O0', '-DEFI_DEBUG'] elif get_option('buildtype') == 'debugoptimized' - compile_args += ['-ggdb', '-Og'] + compile_args += ['-ggdb', '-Og', '-DEFI_DEBUG'] else compile_args += ['-O2'] endif + if get_option('b_ndebug') == 'true' or ( + get_option('b_ndebug') == 'if-release' and ['plain', 'release'].contains(get_option('buildtype'))) + compile_args += ['-DNDEBUG'] + endif efi_ldflags = ['-T', efi_lds, '-shared', diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index 91b24a1826..461d37fa9e 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -50,7 +50,14 @@ #define CONCATENATE(x, y) XCONCATENATE(x, y) #ifdef SD_BOOT -#define assert(expr) do {} while (false) + #ifdef NDEBUG + #define assert(expr) + #define assert_not_reached() __builtin_unreachable() + #else + void efi_assert(const char *expr, const char *file, unsigned line, const char *function) _noreturn_; + #define assert(expr) ({ _likely_(expr) ? VOID_0 : efi_assert(#expr, __FILE__, __LINE__, __PRETTY_FUNCTION__); }) + #define assert_not_reached() efi_assert("Code should not be reached", __FILE__, __LINE__, __PRETTY_FUNCTION__) + #endif #endif #if defined(static_assert) From 508df915b6248cf1322c6a649143ad0dfb7ed166 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 06/10] sd-boot: Assert all the things! --- src/boot/efi/boot.c | 101 +++++++++++++++++++++++++++++++++++++ src/boot/efi/console.c | 6 +++ src/boot/efi/crc32.c | 5 ++ src/boot/efi/disk.c | 2 + src/boot/efi/linux.c | 5 ++ src/boot/efi/measure.c | 9 ++++ src/boot/efi/pe.c | 6 +++ src/boot/efi/random-seed.c | 30 ++++++++--- src/boot/efi/sha256.c | 13 +++++ src/boot/efi/shim.c | 7 +++ src/boot/efi/splash.c | 14 +++++ src/boot/efi/util.c | 56 +++++++++++++++++++- 12 files changed, 247 insertions(+), 7 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 54d704f0d1..e6e1020913 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -72,6 +72,9 @@ typedef struct { } Config; static VOID cursor_left(UINTN *cursor, UINTN *first) { + assert(cursor); + assert(first); + if ((*cursor) > 0) (*cursor)--; else if ((*first) > 0) @@ -84,6 +87,9 @@ static VOID cursor_right( UINTN x_max, UINTN len) { + assert(cursor); + assert(first); + if ((*cursor)+1 < x_max) (*cursor)++; else if ((*first) + (*cursor) < len) @@ -100,6 +106,8 @@ static BOOLEAN line_edit( UINTN size, len, first, cursor, clear; BOOLEAN exit, enter; + assert(line_out); + if (!line_in) line_in = L""; size = StrLen(line_in) + 1024; @@ -332,6 +340,8 @@ static BOOLEAN line_edit( } static UINTN entry_lookup_key(Config *config, UINTN start, CHAR16 key) { + assert(config); + if (key == 0) return -1; @@ -362,6 +372,9 @@ static VOID print_status(Config *config, CHAR16 *loaded_image_path) { _cleanup_freepool_ CHAR16 *partstr = NULL, *defaultstr = NULL; UINTN x, y; + assert(config); + assert(loaded_image_path); + uefi_call_wrapper(ST->ConOut->SetAttribute, 2, ST->ConOut, EFI_LIGHTGRAY|EFI_BACKGROUND_BLACK); uefi_call_wrapper(ST->ConOut->ClearScreen, 1, ST->ConOut); @@ -493,6 +506,10 @@ static BOOLEAN menu_run( ConfigEntry **chosen_entry, CHAR16 *loaded_image_path) { + assert(config); + assert(chosen_entry); + assert(loaded_image_path); + EFI_STATUS err; UINTN visible_max; UINTN idx_highlight; @@ -884,6 +901,9 @@ static BOOLEAN menu_run( } static VOID config_add_entry(Config *config, ConfigEntry *entry) { + assert(config); + assert(entry); + if ((config->entry_count & 15) == 0) { UINTN i; @@ -924,6 +944,12 @@ static CHAR8 *line_get_key_value( CHAR8 *line, *value; UINTN linelen; + assert(content); + assert(sep); + assert(pos); + assert(key_ret); + assert(value_ret); + skip: line = content + *pos; if (*line == '\0') @@ -986,6 +1012,9 @@ static VOID config_defaults_load_from_file(Config *config, CHAR8 *content) { UINTN pos = 0; CHAR8 *key, *value; + assert(config); + assert(content); + while ((line = line_get_key_value(content, (CHAR8 *)" \t", &pos, &key, &value))) { if (strcmpa((CHAR8 *)"timeout", key) == 0) { _cleanup_freepool_ CHAR16 *s = NULL; @@ -1079,6 +1108,10 @@ static VOID config_entry_parse_tries( UINTN left = UINTN_MAX, done = UINTN_MAX, factor = 1, i, next_left, next_done; _cleanup_freepool_ CHAR16 *prefix = NULL; + assert(entry); + assert(path); + assert(file); + /* * Parses a suffix of two counters (one going down, one going up) in the form "+LEFT-DONE" from the end of the * filename (but before the .efi/.conf suffix), where the "-DONE" part is optional and may be left out (in @@ -1196,6 +1229,9 @@ static VOID config_entry_bump_counters( UINTN file_info_size, a, b; EFI_STATUS r; + assert(entry); + assert(root_dir); + if (entry->tries_left == UINTN_MAX) return; @@ -1269,6 +1305,14 @@ static VOID config_entry_add_from_file( EFI_FILE_HANDLE handle; _cleanup_freepool_ CHAR16 *initrd = NULL; + assert(config); + assert(device); + assert(root_dir); + assert(path); + assert(file); + assert(content); + assert(loaded_image_path); + entry = AllocatePool(sizeof(ConfigEntry)); *entry = (ConfigEntry) { @@ -1397,6 +1441,8 @@ static VOID config_load_defaults(Config *config, EFI_FILE *root_dir) { UINTN sec; EFI_STATUS err; + assert(root_dir); + *config = (Config) { .editor = TRUE, .auto_entries = TRUE, @@ -1434,6 +1480,11 @@ static VOID config_load_entries( EFI_FILE_HANDLE entries_dir; EFI_STATUS err; + assert(config); + assert(device); + assert(root_dir); + assert(loaded_image_path); + err = uefi_call_wrapper(root_dir->Open, 5, root_dir, &entries_dir, (CHAR16*) L"\\loader\\entries", EFI_FILE_MODE_READ, 0ULL); if (!EFI_ERROR(err)) { for (;;) { @@ -1469,6 +1520,9 @@ static VOID config_load_entries( static INTN config_entry_compare(ConfigEntry *a, ConfigEntry *b) { INTN r; + assert(a); + assert(b); + /* Order entries that have no tries left to the beginning of the list */ if (a->tries_left != 0 && b->tries_left == 0) return 1; @@ -1499,6 +1553,8 @@ static INTN config_entry_compare(ConfigEntry *a, ConfigEntry *b) { } static VOID config_sort_entries(Config *config) { + assert(config); + for (UINTN i = 1; i < config->entry_count; i++) { BOOLEAN more; @@ -1520,6 +1576,9 @@ static VOID config_sort_entries(Config *config) { } static INTN config_entry_find(Config *config, CHAR16 *id) { + assert(config); + assert(id); + for (UINTN i = 0; i < config->entry_count; i++) if (StrCmp(config->entries[i]->id, id) == 0) return (INTN) i; @@ -1532,6 +1591,8 @@ static VOID config_default_entry_select(Config *config) { EFI_STATUS err; INTN i; + assert(config); + /* * The EFI variable to specify a boot entry for the next, and only the * next reboot. The variable is always cleared directly after it is read. @@ -1602,6 +1663,8 @@ static VOID config_default_entry_select(Config *config) { static BOOLEAN find_nonunique(ConfigEntry **entries, UINTN entry_count) { BOOLEAN non_unique = FALSE; + assert(entries); + for (UINTN i = 0; i < entry_count; i++) entries[i]->non_unique = FALSE; @@ -1620,6 +1683,8 @@ static BOOLEAN find_nonunique(ConfigEntry **entries, UINTN entry_count) { /* generate a unique title, avoiding non-distinguishable menu entries */ static VOID config_title_generate(Config *config) { + assert(config); + /* set title */ for (UINTN i = 0; i < config->entry_count; i++) { CHAR16 *title; @@ -1692,6 +1757,11 @@ static BOOLEAN config_entry_add_call( ConfigEntry *entry; + assert(config); + assert(id); + assert(title); + assert(call); + entry = AllocatePool(sizeof(ConfigEntry)); *entry = (ConfigEntry) { .id = StrDuplicate(id), @@ -1718,6 +1788,12 @@ static ConfigEntry *config_entry_add_loader( ConfigEntry *entry; + assert(config); + assert(device); + assert(id); + assert(title); + assert(loader); + entry = AllocatePool(sizeof(ConfigEntry)); *entry = (ConfigEntry) { .type = type, @@ -1751,6 +1827,13 @@ static BOOLEAN config_entry_add_loader_auto( ConfigEntry *entry; EFI_STATUS err; + assert(config); + assert(device); + assert(root_dir); + assert(id); + assert(title); + assert(loader); + if (!config->auto_entries) return FALSE; @@ -1791,6 +1874,8 @@ static VOID config_entry_add_osx(Config *config) { UINTN handle_count = 0; _cleanup_freepool_ EFI_HANDLE *handles = NULL; + assert(config); + if (!config->auto_entries) return; @@ -1821,6 +1906,10 @@ static VOID config_entry_add_linux( EFI_STATUS err; ConfigEntry *entry; + assert(config); + assert(device); + assert(root_dir); + err = uefi_call_wrapper(root_dir->Open, 5, root_dir, &linux_dir, (CHAR16*) L"\\EFI\\Linux", EFI_FILE_MODE_READ, 0ULL); if (EFI_ERROR(err)) return; @@ -1955,6 +2044,9 @@ static EFI_DEVICE_PATH *path_parent(EFI_DEVICE_PATH *path, EFI_DEVICE_PATH *node EFI_DEVICE_PATH *parent; UINTN len; + assert(path); + assert(node); + len = (UINT8*) NextDevicePathNode(node) - (UINT8*) path; parent = (EFI_DEVICE_PATH*) AllocatePool(len + sizeof(EFI_DEVICE_PATH)); CopyMem(parent, path, len); @@ -1976,6 +2068,9 @@ static VOID config_load_xbootldr( EFI_FILE *root_dir; EFI_STATUS r; + assert(config); + assert(device); + partition_path = DevicePathFromHandle(device); if (!partition_path) return; @@ -2162,6 +2257,9 @@ static EFI_STATUS image_start( CHAR16 *options; EFI_STATUS err; + assert(config); + assert(entry); + path = FileDevicePath(entry->device, entry->loader); if (!path) return log_error_status_stall(EFI_INVALID_PARAMETER, L"Error getting device path."); @@ -2224,6 +2322,7 @@ static EFI_STATUS reboot_into_firmware(VOID) { } static VOID config_free(Config *config) { + assert(config); for (UINTN i = 0; i < config->entry_count; i++) config_entry_free(config->entries[i]); FreePool(config->entries); @@ -2237,6 +2336,8 @@ static VOID config_write_entries_to_variable(Config *config) { UINTN sz = 0; CHAR16 *p; + assert(config); + for (UINTN i = 0; i < config->entry_count; i++) sz += StrLen(config->entries[i]->id) + 1; diff --git a/src/boot/efi/console.c b/src/boot/efi/console.c index 83619d2147..532234030b 100644 --- a/src/boot/efi/console.c +++ b/src/boot/efi/console.c @@ -18,6 +18,8 @@ EFI_STATUS console_key_read(UINT64 *key, BOOLEAN wait) { EFI_INPUT_KEY k; EFI_STATUS err; + assert(key); + if (!checked) { err = LibLocateProtocol(EFI_SIMPLE_TEXT_INPUT_EX_GUID, (VOID **)&TextInputEx); if (EFI_ERROR(err)) @@ -108,6 +110,8 @@ static EFI_STATUS mode_auto(UINTN *mode) { EFI_STATUS err; BOOLEAN keep = FALSE; + assert(mode); + err = LibLocateProtocol(&GraphicsOutputProtocolGuid, (VOID **)&GraphicsOutput); if (!EFI_ERROR(err) && GraphicsOutput->Mode && GraphicsOutput->Mode->Info) { Info = GraphicsOutput->Mode->Info; @@ -156,6 +160,8 @@ static EFI_STATUS mode_auto(UINTN *mode) { } EFI_STATUS console_set_mode(UINTN *mode, enum console_mode_change_type how) { + assert(mode); + if (how == CONSOLE_MODE_AUTO) return mode_auto(mode); diff --git a/src/boot/efi/crc32.c b/src/boot/efi/crc32.c index c9e5501ffa..5130ae73d5 100644 --- a/src/boot/efi/crc32.c +++ b/src/boot/efi/crc32.c @@ -42,6 +42,7 @@ */ #include "crc32.h" +#include "macro-fundamental.h" static const UINT32 crc32_tab[] = { 0x00000000L, 0x77073096L, 0xee0e612cL, 0x990951baL, 0x076dc419L, @@ -111,6 +112,8 @@ UINT32 crc32(UINT32 seed, const VOID *buf, UINTN len) { const UINT8 *p = buf; UINT32 crc = seed; + assert(buf); + while (len > 0) { crc = crc32_add_char(crc, *p++); len--; @@ -129,6 +132,8 @@ UINT32 crc32_exclude_offset( const UINT8 *p = buf; UINT32 crc = seed; + assert(buf); + for (UINTN i = 0; i < len; i++) { UINT8 x = *p++; diff --git a/src/boot/efi/disk.c b/src/boot/efi/disk.c index 122f08dd5c..196dc52be5 100644 --- a/src/boot/efi/disk.c +++ b/src/boot/efi/disk.c @@ -9,6 +9,8 @@ EFI_STATUS disk_get_part_uuid(EFI_HANDLE *handle, CHAR16 uuid[static 37]) { EFI_DEVICE_PATH *device_path; + assert(handle); + /* export the device path this image is started from */ device_path = DevicePathFromHandle(handle); if (device_path) { diff --git a/src/boot/efi/linux.c b/src/boot/efi/linux.c index 4d44671315..748ca29f7e 100644 --- a/src/boot/efi/linux.c +++ b/src/boot/efi/linux.c @@ -17,6 +17,8 @@ static VOID linux_efi_handover(EFI_HANDLE image, struct boot_params *params) { handover_f handover; UINTN start = (UINTN)params->hdr.code32_start; + assert(params); + #ifdef __x86_64__ asm volatile ("cli"); start += 512; @@ -35,6 +37,9 @@ EFI_STATUS linux_exec(EFI_HANDLE *image, EFI_PHYSICAL_ADDRESS addr; EFI_STATUS err; + assert(image); + assert(cmdline); + image_params = (struct boot_params *) linux_addr; if (image_params->hdr.boot_flag != 0xAA55 || diff --git a/src/boot/efi/measure.c b/src/boot/efi/measure.c index c272d08553..4a8a2c7cce 100644 --- a/src/boot/efi/measure.c +++ b/src/boot/efi/measure.c @@ -5,6 +5,7 @@ #include #include +#include "macro-fundamental.h" #include "measure.h" #define EFI_TCG_GUID \ @@ -184,6 +185,9 @@ static EFI_STATUS tpm1_measure_to_pcr_and_event_log(const EFI_TCG *tcg, UINT32 p EFI_PHYSICAL_ADDRESS event_log_last; UINTN desc_len; + assert(tcg); + assert(description); + desc_len = (StrLen(description) + 1) * sizeof(CHAR16); tcg_event = AllocateZeroPool(desc_len + sizeof(TCG_PCR_EVENT)); @@ -215,6 +219,9 @@ static EFI_STATUS tpm2_measure_to_pcr_and_event_log(const EFI_TCG2 *tcg, UINT32 EFI_TCG2_EVENT *tcg_event; UINTN desc_len; + assert(tcg); + assert(description); + desc_len = StrLen(description) * sizeof(CHAR16); tcg_event = AllocateZeroPool(sizeof(*tcg_event) - sizeof(tcg_event->Event) + desc_len + 1); @@ -302,6 +309,8 @@ EFI_STATUS tpm_log_event(UINT32 pcrindex, const EFI_PHYSICAL_ADDRESS buffer, UIN EFI_TCG *tpm1; EFI_TCG2 *tpm2; + assert(description); + tpm2 = tcg2_interface_check(); if (tpm2) return tpm2_measure_to_pcr_and_event_log(tpm2, pcrindex, buffer, buffer_size, description); diff --git a/src/boot/efi/pe.c b/src/boot/efi/pe.c index e657017343..5c20acc9f3 100644 --- a/src/boot/efi/pe.c +++ b/src/boot/efi/pe.c @@ -66,6 +66,9 @@ EFI_STATUS pe_memory_locate_sections(CHAR8 *base, CHAR8 **sections, UINTN *addrs struct PeHeader *pe; UINTN offset; + assert(base); + assert(sections); + dos = (struct DosFileHeader *)base; if (CompareMem(dos->Magic, "MZ", 2) != 0) @@ -118,6 +121,9 @@ EFI_STATUS pe_file_locate_sections(EFI_FILE *dir, CHAR16 *path, CHAR8 **sections EFI_STATUS err; _cleanup_freepool_ CHAR8 *header = NULL; + assert(dir); + assert(path); + err = uefi_call_wrapper(dir->Open, 5, dir, &handle, path, EFI_FILE_MODE_READ, 0ULL); if (EFI_ERROR(err)) return err; diff --git a/src/boot/efi/random-seed.c b/src/boot/efi/random-seed.c index 939daf3e41..55348a64ec 100644 --- a/src/boot/efi/random-seed.c +++ b/src/boot/efi/random-seed.c @@ -22,6 +22,8 @@ static EFI_STATUS acquire_rng(UINTN size, VOID **ret) { EFI_RNG_PROTOCOL *rng; EFI_STATUS err; + assert(ret); + /* Try to acquire the specified number of bytes from the UEFI RNG */ err = LibLocateProtocol(EFI_RNG_GUID, (VOID**) &rng); @@ -63,6 +65,10 @@ static VOID hash_once( struct sha256_ctx hash; + assert(old_seed); + assert(rng); + assert(system_token); + sha256_init_ctx(&hash); sha256_process_bytes(old_seed, size, &hash); if (rng) @@ -85,6 +91,11 @@ static EFI_STATUS hash_many( _cleanup_freepool_ VOID *output = NULL; + assert(old_seed); + assert(rng); + assert(system_token); + assert(ret); + /* Hashes the specified parameters in counter mode, generating n hash values, with the counter in the * range counter_start…counter_start+n-1. */ @@ -115,6 +126,12 @@ static EFI_STATUS mangle_random_seed( EFI_STATUS err; UINTN n; + assert(old_seed); + assert(rng); + assert(system_token); + assert(ret_new_seed); + assert(ret_for_kernel); + /* This takes the old seed file contents, an (optional) random number acquired from the UEFI RNG, an * (optional) system 'token' installed once by the OS installer in an EFI variable, and hashes them * together in counter mode, generating a new seed (to replace the file on disk) and the seed for the @@ -144,6 +161,9 @@ static EFI_STATUS acquire_system_token(VOID **ret, UINTN *ret_size) { EFI_STATUS err; UINTN size; + assert(ret); + assert(ret_size); + err = efivar_get_raw(LOADER_GUID, L"LoaderSystemToken", &data, &size); if (EFI_ERROR(err)) { if (err != EFI_NOT_FOUND) @@ -162,7 +182,7 @@ static EFI_STATUS acquire_system_token(VOID **ret, UINTN *ret_size) { static VOID validate_sha256(void) { -#ifndef __OPTIMIZE__ +#ifdef EFI_DEBUG /* Let's validate our SHA256 implementation. We stole it from glibc, and converted it to UEFI * style. We better check whether it does the right stuff. We use the simpler test vectors from the * SHA spec. Note that we strip this out in optimization builds. */ @@ -204,13 +224,9 @@ static VOID validate_sha256(void) { sha256_process_bytes(array[i].string, strlena((const CHAR8*) array[i].string), &hash); sha256_finish_ctx(&hash, result); - if (CompareMem(result, array[i].hash, HASH_VALUE_SIZE) != 0) { - log_error_stall(L"SHA256 failed validation."); - return; - } + assert(CompareMem(result, array[i].hash, HASH_VALUE_SIZE) == 0); } - Print(L"SHA256 validated\n"); #endif } @@ -221,6 +237,8 @@ EFI_STATUS process_random_seed(EFI_FILE *root_dir, RandomSeedMode mode) { _cleanup_freepool_ EFI_FILE_INFO *info = NULL; EFI_STATUS err; + assert(root_dir); + validate_sha256(); if (mode == RANDOM_SEED_OFF) diff --git a/src/boot/efi/sha256.c b/src/boot/efi/sha256.c index 6585fdb9b2..5df7e5316e 100644 --- a/src/boot/efi/sha256.c +++ b/src/boot/efi/sha256.c @@ -23,6 +23,7 @@ /* Written by Ulrich Drepper , 2007. */ +#include "macro-fundamental.h" #include "sha256.h" #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ @@ -73,6 +74,8 @@ static void sha256_process_block(const void *, UINTN, struct sha256_ctx *); /* Initialize structure containing state of computation. (FIPS 180-2:5.3.2) */ void sha256_init_ctx(struct sha256_ctx *ctx) { + assert(ctx); + ctx->H[0] = 0x6a09e667; ctx->H[1] = 0xbb67ae85; ctx->H[2] = 0x3c6ef372; @@ -96,6 +99,9 @@ void *sha256_finish_ctx(struct sha256_ctx *ctx, void *resbuf) { UINT32 bytes = ctx->buflen; UINTN pad; + assert(ctx); + assert(resbuf); + /* Now count remaining bytes. */ ctx->total64 += bytes; @@ -118,6 +124,9 @@ void *sha256_finish_ctx(struct sha256_ctx *ctx, void *resbuf) { } void sha256_process_bytes(const void *buffer, UINTN len, struct sha256_ctx *ctx) { + assert(buffer); + assert(ctx); + /* When we already have some bits in our internal buffer concatenate both inputs first. */ @@ -191,6 +200,10 @@ void sha256_process_bytes(const void *buffer, UINTN len, struct sha256_ctx *ctx) static void sha256_process_block(const void *buffer, UINTN len, struct sha256_ctx *ctx) { const UINT32 *words = buffer; UINTN nwords = len / sizeof (UINT32); + + assert(buffer); + assert(ctx); + UINT32 a = ctx->H[0]; UINT32 b = ctx->H[1]; UINT32 c = ctx->H[2]; diff --git a/src/boot/efi/shim.c b/src/boot/efi/shim.c index 48602627bb..9bcbb34769 100644 --- a/src/boot/efi/shim.c +++ b/src/boot/efi/shim.c @@ -111,6 +111,10 @@ static EFIAPI EFI_STATUS security2_policy_authentication (const EFI_SECURITY2_PR VOID *file_buffer, UINTN file_size, BOOLEAN boot_policy) { EFI_STATUS status; + assert(this); + assert(device_path); + assert(file_buffer); + /* Chain original security policy */ status = uefi_call_wrapper(es2fa, 5, this, device_path, file_buffer, file_size, boot_policy); @@ -143,6 +147,9 @@ static EFIAPI EFI_STATUS security_policy_authentication (const EFI_SECURITY_PROT _cleanup_freepool_ CHAR8 *file_buffer = NULL; UINTN file_size; + assert(this); + assert(device_path_const); + if (!device_path_const) return EFI_INVALID_PARAMETER; diff --git a/src/boot/efi/splash.c b/src/boot/efi/splash.c index 5e085ec45c..920b440825 100644 --- a/src/boot/efi/splash.c +++ b/src/boot/efi/splash.c @@ -44,6 +44,11 @@ static EFI_STATUS bmp_parse_header(UINT8 *bmp, UINTN size, struct bmp_dib **ret_ struct bmp_map *map; UINTN row_size; + assert(bmp); + assert(ret_dib); + assert(ret_map); + assert(pixmap); + if (size < sizeof(struct bmp_file) + sizeof(struct bmp_dib)) return EFI_INVALID_PARAMETER; @@ -128,6 +133,8 @@ static EFI_STATUS bmp_parse_header(UINT8 *bmp, UINTN size, struct bmp_dib **ret_ static VOID pixel_blend(UINT32 *dst, const UINT32 source) { UINT32 alpha, src, src_rb, src_g, dst_rb, dst_g, rb, g; + assert(dst); + alpha = (source & 0xff); /* convert src from RGBA to XRGB */ @@ -152,6 +159,11 @@ static EFI_STATUS bmp_to_blt(EFI_GRAPHICS_OUTPUT_BLT_PIXEL *buf, UINT8 *pixmap) { UINT8 *in; + assert(buf); + assert(dib); + assert(map); + assert(pixmap); + /* transform and copy pixels */ in = pixmap; for (UINTN y = 0; y < dib->y; y++) { @@ -247,6 +259,8 @@ EFI_STATUS graphics_splash(UINT8 *content, UINTN len, const EFI_GRAPHICS_OUTPUT_ UINTN y_pos = 0; EFI_STATUS err; + assert(content); + if (!background) { if (StriCmp(L"Apple", ST->FirmwareVendor) == 0) { pixel.Red = 0xc0; diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index b048df0764..932a4cfa85 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -67,6 +67,8 @@ UINT64 time_usec(VOID) { } EFI_STATUS parse_boolean(const CHAR8 *v, BOOLEAN *b) { + assert(b); + if (!v) return EFI_INVALID_PARAMETER; @@ -90,17 +92,27 @@ EFI_STATUS parse_boolean(const CHAR8 *v, BOOLEAN *b) { } EFI_STATUS efivar_set_raw(const EFI_GUID *vendor, const CHAR16 *name, const VOID *buf, UINTN size, UINT32 flags) { + assert(vendor); + assert(name); + assert(buf || size == 0); + flags |= EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS; return uefi_call_wrapper(RT->SetVariable, 5, (CHAR16*) name, (EFI_GUID *)vendor, flags, size, (VOID*) buf); } EFI_STATUS efivar_set(const EFI_GUID *vendor, const CHAR16 *name, const CHAR16 *value, UINT32 flags) { + assert(vendor); + assert(name); + return efivar_set_raw(vendor, name, value, value ? (StrLen(value) + 1) * sizeof(CHAR16) : 0, flags); } EFI_STATUS efivar_set_uint_string(const EFI_GUID *vendor, const CHAR16 *name, UINTN i, UINT32 flags) { CHAR16 str[32]; + assert(vendor); + assert(name); + SPrint(str, ELEMENTSOF(str), L"%u", i); return efivar_set(vendor, name, str, flags); } @@ -108,6 +120,9 @@ EFI_STATUS efivar_set_uint_string(const EFI_GUID *vendor, const CHAR16 *name, UI EFI_STATUS efivar_set_uint32_le(const EFI_GUID *vendor, const CHAR16 *name, UINT32 value, UINT32 flags) { UINT8 buf[4]; + assert(vendor); + assert(name); + buf[0] = (UINT8)(value >> 0U & 0xFF); buf[1] = (UINT8)(value >> 8U & 0xFF); buf[2] = (UINT8)(value >> 16U & 0xFF); @@ -119,6 +134,9 @@ EFI_STATUS efivar_set_uint32_le(const EFI_GUID *vendor, const CHAR16 *name, UINT EFI_STATUS efivar_set_uint64_le(const EFI_GUID *vendor, const CHAR16 *name, UINT64 value, UINT32 flags) { UINT8 buf[8]; + assert(vendor); + assert(name); + buf[0] = (UINT8)(value >> 0U & 0xFF); buf[1] = (UINT8)(value >> 8U & 0xFF); buf[2] = (UINT8)(value >> 16U & 0xFF); @@ -137,6 +155,9 @@ EFI_STATUS efivar_get(const EFI_GUID *vendor, const CHAR16 *name, CHAR16 **value CHAR16 *val; UINTN size; + assert(vendor); + assert(name); + err = efivar_get_raw(vendor, name, (CHAR8**)&buf, &size); if (EFI_ERROR(err)) return err; @@ -170,8 +191,12 @@ EFI_STATUS efivar_get_uint_string(const EFI_GUID *vendor, const CHAR16 *name, UI _cleanup_freepool_ CHAR16 *val = NULL; EFI_STATUS err; + assert(vendor); + assert(name); + assert(i); + err = efivar_get(vendor, name, &val); - if (!EFI_ERROR(err) && i) + if (!EFI_ERROR(err)) *i = Atoi(val); return err; @@ -182,6 +207,9 @@ EFI_STATUS efivar_get_uint32_le(const EFI_GUID *vendor, const CHAR16 *name, UINT UINTN size; EFI_STATUS err; + assert(vendor); + assert(name); + err = efivar_get_raw(vendor, name, &buf, &size); if (!EFI_ERROR(err) && ret) { if (size != sizeof(UINT32)) @@ -199,6 +227,9 @@ EFI_STATUS efivar_get_uint64_le(const EFI_GUID *vendor, const CHAR16 *name, UINT UINTN size; EFI_STATUS err; + assert(vendor); + assert(name); + err = efivar_get_raw(vendor, name, &buf, &size); if (!EFI_ERROR(err) && ret) { if (size != sizeof(UINT64)) @@ -217,6 +248,9 @@ EFI_STATUS efivar_get_raw(const EFI_GUID *vendor, const CHAR16 *name, CHAR8 **bu UINTN l; EFI_STATUS err; + assert(vendor); + assert(name); + l = sizeof(CHAR16 *) * EFI_MAXIMUM_VARIABLE_SIZE; buf = AllocatePool(l); if (!buf) @@ -240,6 +274,10 @@ EFI_STATUS efivar_get_boolean_u8(const EFI_GUID *vendor, const CHAR16 *name, BOO UINTN size; EFI_STATUS err; + assert(vendor); + assert(name); + assert(ret); + err = efivar_get_raw(vendor, name, &b, &size); if (!EFI_ERROR(err)) *ret = *b > 0; @@ -250,6 +288,9 @@ EFI_STATUS efivar_get_boolean_u8(const EFI_GUID *vendor, const CHAR16 *name, BOO VOID efivar_set_time_usec(const EFI_GUID *vendor, const CHAR16 *name, UINT64 usec) { CHAR16 str[32]; + assert(vendor); + assert(name); + if (usec == 0) usec = time_usec(); if (usec == 0) @@ -263,6 +304,9 @@ static INTN utf8_to_16(const CHAR8 *stra, CHAR16 *c) { CHAR16 unichar; UINTN len; + assert(stra); + assert(c); + if (!(stra[0] & 0x80)) len = 1; else if ((stra[0] & 0xe0) == 0xc0) @@ -316,6 +360,8 @@ CHAR16 *stra_to_str(const CHAR8 *stra) { UINTN i; CHAR16 *str; + assert(stra); + len = strlena(stra); str = AllocatePool((len + 1) * sizeof(CHAR16)); @@ -344,6 +390,8 @@ CHAR16 *stra_to_path(const CHAR8 *stra) { UINTN len; UINTN i; + assert(stra); + len = strlena(stra); str = AllocatePool((len + 2) * sizeof(CHAR16)); @@ -376,6 +424,7 @@ CHAR16 *stra_to_path(const CHAR8 *stra) { } CHAR8 *strchra(const CHAR8 *s, CHAR8 c) { + assert(s); do { if (*s == c) return (CHAR8*) s; @@ -388,6 +437,9 @@ EFI_STATUS file_read(EFI_FILE_HANDLE dir, const CHAR16 *name, UINTN off, UINTN s _cleanup_freepool_ CHAR8 *buf = NULL; EFI_STATUS err; + assert(name); + assert(ret); + err = uefi_call_wrapper(dir->Open, 5, dir, &handle, (CHAR16*) name, EFI_FILE_MODE_READ, 0ULL); if (EFI_ERROR(err)) return err; @@ -428,6 +480,8 @@ EFI_STATUS file_read(EFI_FILE_HANDLE dir, const CHAR16 *name, UINTN off, UINTN s VOID log_error_stall(const CHAR16 *fmt, ...) { va_list args; + assert(fmt); + uefi_call_wrapper(ST->ConOut->SetAttribute, 2, ST->ConOut, EFI_LIGHTRED|EFI_BACKGROUND_BLACK); Print(L"\n"); From b3fc3a3ced7b9604efbd5d90b70403880e8179bb Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 07/10] sd-boot: Use StrSize where it makes sense --- src/boot/efi/boot.c | 18 ++++++++++-------- src/boot/efi/measure.c | 10 ++++------ src/boot/efi/util.c | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index e6e1020913..ab29fb94bf 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -2284,7 +2284,7 @@ static EFI_STATUS image_start( goto out_unload; } loaded_image->LoadOptions = options; - loaded_image->LoadOptionsSize = (StrLen(loaded_image->LoadOptions)+1) * sizeof(CHAR16); + loaded_image->LoadOptionsSize = StrSize(loaded_image->LoadOptions); #if ENABLE_TPM /* Try to log any options to the TPM, especially to catch manually edited options */ @@ -2332,28 +2332,30 @@ static VOID config_free(Config *config) { } static VOID config_write_entries_to_variable(Config *config) { - _cleanup_freepool_ CHAR16 *buffer = NULL; + _cleanup_freepool_ CHAR8 *buffer = NULL; UINTN sz = 0; - CHAR16 *p; + CHAR8 *p; assert(config); for (UINTN i = 0; i < config->entry_count; i++) - sz += StrLen(config->entries[i]->id) + 1; + sz += StrSize(config->entries[i]->id); - p = buffer = AllocatePool(sz * sizeof(CHAR16)); + p = buffer = AllocatePool(sz); for (UINTN i = 0; i < config->entry_count; i++) { UINTN l; - l = StrLen(config->entries[i]->id) + 1; - CopyMem(p, config->entries[i]->id, l * sizeof(CHAR16)); + l = StrSize(config->entries[i]->id); + CopyMem(p, config->entries[i]->id, l); p += l; } + assert(p == buffer + sz); + /* Store the full list of discovered entries. */ - (void) efivar_set_raw(LOADER_GUID, L"LoaderEntries", buffer, (UINT8 *) p - (UINT8 *) buffer, 0); + (void) efivar_set_raw(LOADER_GUID, L"LoaderEntries", buffer, sz, 0); } EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) { diff --git a/src/boot/efi/measure.c b/src/boot/efi/measure.c index 4a8a2c7cce..429e82657b 100644 --- a/src/boot/efi/measure.c +++ b/src/boot/efi/measure.c @@ -188,8 +188,7 @@ static EFI_STATUS tpm1_measure_to_pcr_and_event_log(const EFI_TCG *tcg, UINT32 p assert(tcg); assert(description); - desc_len = (StrLen(description) + 1) * sizeof(CHAR16); - + desc_len = StrSize(description); tcg_event = AllocateZeroPool(desc_len + sizeof(TCG_PCR_EVENT)); if (!tcg_event) @@ -222,14 +221,13 @@ static EFI_STATUS tpm2_measure_to_pcr_and_event_log(const EFI_TCG2 *tcg, UINT32 assert(tcg); assert(description); - desc_len = StrLen(description) * sizeof(CHAR16); - - tcg_event = AllocateZeroPool(sizeof(*tcg_event) - sizeof(tcg_event->Event) + desc_len + 1); + desc_len = StrSize(description); + tcg_event = AllocateZeroPool(sizeof(*tcg_event) - sizeof(tcg_event->Event) + desc_len); if (!tcg_event) return EFI_OUT_OF_RESOURCES; - tcg_event->Size = sizeof(*tcg_event) - sizeof(tcg_event->Event) + desc_len + 1; + tcg_event->Size = sizeof(*tcg_event) - sizeof(tcg_event->Event) + desc_len; tcg_event->Header.HeaderSize = sizeof(EFI_TCG2_EVENT_HEADER); tcg_event->Header.HeaderVersion = EFI_TCG2_EVENT_HEADER_VERSION; tcg_event->Header.PCRIndex = pcrindex; diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index 932a4cfa85..b2977546a8 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -104,7 +104,7 @@ EFI_STATUS efivar_set(const EFI_GUID *vendor, const CHAR16 *name, const CHAR16 * assert(vendor); assert(name); - return efivar_set_raw(vendor, name, value, value ? (StrLen(value) + 1) * sizeof(CHAR16) : 0, flags); + return efivar_set_raw(vendor, name, value, value ? StrSize(value) : 0, flags); } EFI_STATUS efivar_set_uint_string(const EFI_GUID *vendor, const CHAR16 *name, UINTN i, UINT32 flags) { From e98d271e57f3d0356e444b6ea2d48836ee2769b0 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 08/10] sd-boot: Rework console input handling Fixes: #15847 Probably fixes: #19191 --- src/boot/efi/boot.c | 55 +++++++--------------- src/boot/efi/console.c | 102 +++++++++++++++++++++++++++++------------ src/boot/efi/console.h | 2 +- 3 files changed, 91 insertions(+), 68 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index ab29fb94bf..96099e5739 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -142,7 +142,7 @@ static BOOLEAN line_edit( uefi_call_wrapper(ST->ConOut->OutputString, 2, ST->ConOut, print); uefi_call_wrapper(ST->ConOut->SetCursorPosition, 3, ST->ConOut, cursor, y_pos); - err = console_key_read(&key, TRUE); + err = console_key_read(&key, 0); if (EFI_ERROR(err)) continue; @@ -400,7 +400,7 @@ static VOID print_status(Config *config, CHAR16 *loaded_image_path) { Print(L"OsIndicationsSupported: %d\n", indvar); Print(L"\n--- press key ---\n\n"); - console_key_read(&key, TRUE); + console_key_read(&key, 0); Print(L"timeout: %u\n", config->timeout_sec); if (config->timeout_sec_efivar >= 0) @@ -445,7 +445,7 @@ static VOID print_status(Config *config, CHAR16 *loaded_image_path) { Print(L"LoaderEntryDefault: %s\n", defaultstr); Print(L"\n--- press key ---\n\n"); - console_key_read(&key, TRUE); + console_key_read(&key, 0); for (UINTN i = 0; i < config->entry_count; i++) { ConfigEntry *entry; @@ -495,7 +495,7 @@ static VOID print_status(Config *config, CHAR16 *loaded_image_path) { entry->path, entry->next_name); Print(L"\n--- press key ---\n\n"); - console_key_read(&key, TRUE); + console_key_read(&key, 0); } uefi_call_wrapper(ST->ConOut->ClearScreen, 1, ST->ConOut); @@ -526,11 +526,10 @@ static BOOLEAN menu_run( UINTN y_max; CHAR16 *status; CHAR16 *clearline; - INTN timeout_remain; + UINTN timeout_remain = config->timeout_sec; INT16 idx; BOOLEAN exit = FALSE; BOOLEAN run = TRUE; - BOOLEAN wait = FALSE; graphics_mode(FALSE); uefi_call_wrapper(ST->ConIn->Reset, 2, ST->ConIn, FALSE); @@ -555,12 +554,6 @@ static BOOLEAN menu_run( y_max = 25; } - /* we check 10 times per second for a keystroke */ - if (config->timeout_sec > 0) - timeout_remain = config->timeout_sec * 10; - else - timeout_remain = -1; - idx_highlight = config->idx_default; idx_highlight_prev = 0; @@ -660,7 +653,7 @@ static BOOLEAN menu_run( if (timeout_remain > 0) { FreePool(status); - status = PoolPrint(L"Boot in %d sec.", (timeout_remain + 5) / 10); + status = PoolPrint(L"Boot in %d s.", timeout_remain); } /* print status at last line of screen */ @@ -681,27 +674,18 @@ static BOOLEAN menu_run( uefi_call_wrapper(ST->ConOut->OutputString, 2, ST->ConOut, clearline+1 + x + len); } - err = console_key_read(&key, wait); - if (EFI_ERROR(err)) { - /* timeout reached */ + err = console_key_read(&key, timeout_remain > 0 ? 1000 * 1000 : 0); + if (err == EFI_TIMEOUT) { + timeout_remain--; if (timeout_remain == 0) { exit = TRUE; break; } - /* sleep and update status */ - if (timeout_remain > 0) { - uefi_call_wrapper(BS->Stall, 1, 100 * 1000); - timeout_remain--; - continue; - } - - /* timeout disabled, wait for next key */ - wait = TRUE; + /* update status */ continue; - } - - timeout_remain = -1; + } else + timeout_remain = 0; /* clear status after keystroke */ if (status) { @@ -804,7 +788,7 @@ static BOOLEAN menu_run( config->timeout_sec_efivar, EFI_VARIABLE_NON_VOLATILE); if (config->timeout_sec_efivar > 0) - status = PoolPrint(L"Menu timeout set to %d sec.", config->timeout_sec_efivar); + status = PoolPrint(L"Menu timeout set to %d s.", config->timeout_sec_efivar); else status = StrDuplicate(L"Menu disabled. Hold down key at bootup to show menu."); } else if (config->timeout_sec_efivar <= 0){ @@ -812,7 +796,7 @@ static BOOLEAN menu_run( efivar_set( LOADER_GUID, L"LoaderConfigTimeout", NULL, EFI_VARIABLE_NON_VOLATILE); if (config->timeout_sec_config > 0) - status = PoolPrint(L"Menu timeout of %d sec is defined by configuration file.", + status = PoolPrint(L"Menu timeout of %d s is defined by configuration file.", config->timeout_sec_config); else status = StrDuplicate(L"Menu disabled. Hold down key at bootup to show menu."); @@ -830,7 +814,7 @@ static BOOLEAN menu_run( config->timeout_sec_efivar, EFI_VARIABLE_NON_VOLATILE); if (config->timeout_sec_efivar > 0) - status = PoolPrint(L"Menu timeout set to %d sec.", + status = PoolPrint(L"Menu timeout set to %d s.", config->timeout_sec_efivar); else status = StrDuplicate(L"Menu disabled. Hold down key at bootup to show menu."); @@ -2472,13 +2456,8 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) { else { UINT64 key; - err = console_key_read(&key, FALSE); - - if (err == EFI_NOT_READY) { - uefi_call_wrapper(BS->Stall, 1, 100 * 1000); - err = console_key_read(&key, FALSE); - } - + /* Block up to 100ms to give firmware time to get input working. */ + err = console_key_read(&key, 100 * 1000); if (!EFI_ERROR(err)) { INT16 idx; diff --git a/src/boot/efi/console.c b/src/boot/efi/console.c index 532234030b..3f405113bd 100644 --- a/src/boot/efi/console.c +++ b/src/boot/efi/console.c @@ -11,63 +11,107 @@ #define EFI_SIMPLE_TEXT_INPUT_EX_GUID &(EFI_GUID) EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID -EFI_STATUS console_key_read(UINT64 *key, BOOLEAN wait) { +static inline void EventClosep(EFI_EVENT *event) { + if (!*event) + return; + + uefi_call_wrapper(BS->CloseEvent, 1, *event); +} + +/* + * Reading input from the console sounds like an easy task to do, but thanks to broken + * firmware it is actually a nightmare. + * + * There is a ConIn and TextInputEx API for this. Ideally we want to use TextInputEx, + * because that gives us Ctrl/Alt/Shift key state information. Unfortunately, it is not + * always available and sometimes just non-functional. + * + * On the other hand we have ConIn, where some firmware likes to just freeze on us + * if we call ReadKeyStroke on it. + * + * Therefore, we use WaitForEvent on both ConIn and TextInputEx (if available) along + * with a timer event. The timer ensures there is no need to call into functions + * that might freeze on us, while still allowing us to show a timeout counter. + */ +EFI_STATUS console_key_read(UINT64 *key, UINT64 timeout_usec) { static EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL *TextInputEx; static BOOLEAN checked; UINTN index; EFI_INPUT_KEY k; EFI_STATUS err; + _cleanup_(EventClosep) EFI_EVENT timer = NULL; + EFI_EVENT events[3] = { ST->ConIn->WaitForKey }; + UINTN n_events = 1; assert(key); if (!checked) { err = LibLocateProtocol(EFI_SIMPLE_TEXT_INPUT_EX_GUID, (VOID **)&TextInputEx); - if (EFI_ERROR(err)) + if (EFI_ERROR(err) || + uefi_call_wrapper(BS->CheckEvent, 1, TextInputEx->WaitForKeyEx) == EFI_INVALID_PARAMETER) + /* If WaitForKeyEx fails here, the firmware pretends it talks this + * protocol, but it really doesn't. */ TextInputEx = NULL; + else + events[n_events++] = TextInputEx->WaitForKeyEx; checked = TRUE; } - /* wait until key is pressed */ - if (wait) - uefi_call_wrapper(BS->WaitForEvent, 3, 1, &ST->ConIn->WaitForKey, &index); + if (timeout_usec > 0) { + err = uefi_call_wrapper(BS->CreateEvent, 5, EVT_TIMER, 0, NULL, NULL, &timer); + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Error creating timer event: %r", err); - if (TextInputEx) { - EFI_KEY_DATA keydata; - UINT64 keypress; + /* SetTimer expects 100ns units for some reason. */ + err = uefi_call_wrapper(BS->SetTimer, 3, timer, TimerRelative, timeout_usec * 10); + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Error arming timer event: %r", err); + events[n_events++] = timer; + } + + err = uefi_call_wrapper(BS->WaitForEvent, 3, n_events, events, &index); + if (EFI_ERROR(err)) + return log_error_status_stall(err, L"Error waiting for events: %r", err); + + if (timeout_usec > 0 && timer == events[index]) + return EFI_TIMEOUT; + + /* TextInputEx might be ready too even if ConIn got to signal first. */ + if (TextInputEx && !EFI_ERROR(uefi_call_wrapper(BS->CheckEvent, 1, TextInputEx->WaitForKeyEx))) { + EFI_KEY_DATA keydata; + UINT64 keypress; + UINT32 shift = 0; + err = uefi_call_wrapper(TextInputEx->ReadKeyStrokeEx, 2, TextInputEx, &keydata); - if (!EFI_ERROR(err)) { - UINT32 shift = 0; + if (EFI_ERROR(err)) + return err; - /* do not distinguish between left and right keys */ - if (keydata.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) { - if (keydata.KeyState.KeyShiftState & (EFI_RIGHT_CONTROL_PRESSED|EFI_LEFT_CONTROL_PRESSED)) - shift |= EFI_CONTROL_PRESSED; - if (keydata.KeyState.KeyShiftState & (EFI_RIGHT_ALT_PRESSED|EFI_LEFT_ALT_PRESSED)) - shift |= EFI_ALT_PRESSED; - }; + /* do not distinguish between left and right keys */ + if (keydata.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) { + if (keydata.KeyState.KeyShiftState & (EFI_RIGHT_CONTROL_PRESSED|EFI_LEFT_CONTROL_PRESSED)) + shift |= EFI_CONTROL_PRESSED; + if (keydata.KeyState.KeyShiftState & (EFI_RIGHT_ALT_PRESSED|EFI_LEFT_ALT_PRESSED)) + shift |= EFI_ALT_PRESSED; + }; - /* 32 bit modifier keys + 16 bit scan code + 16 bit unicode */ - keypress = KEYPRESS(shift, keydata.Key.ScanCode, keydata.Key.UnicodeChar); - if (keypress > 0) { - *key = keypress; - return 0; - } + /* 32 bit modifier keys + 16 bit scan code + 16 bit unicode */ + keypress = KEYPRESS(shift, keydata.Key.ScanCode, keydata.Key.UnicodeChar); + if (keypress > 0) { + *key = keypress; + return EFI_SUCCESS; } + + return EFI_NOT_READY; } - /* fallback for firmware which does not support SimpleTextInputExProtocol - * - * This is also called in case ReadKeyStrokeEx did not return a key, because - * some broken firmwares offer SimpleTextInputExProtocol, but never actually - * handle any key. */ err = uefi_call_wrapper(ST->ConIn->ReadKeyStroke, 2, ST->ConIn, &k); if (EFI_ERROR(err)) return err; *key = KEYPRESS(0, k.ScanCode, k.UnicodeChar); - return 0; + return EFI_SUCCESS; } static EFI_STATUS change_mode(UINTN mode) { diff --git a/src/boot/efi/console.h b/src/boot/efi/console.h index 2c69af552a..23848a9c58 100644 --- a/src/boot/efi/console.h +++ b/src/boot/efi/console.h @@ -16,5 +16,5 @@ enum console_mode_change_type { CONSOLE_MODE_MAX, }; -EFI_STATUS console_key_read(UINT64 *key, BOOLEAN wait); +EFI_STATUS console_key_read(UINT64 *key, UINT64 timeout_usec); EFI_STATUS console_set_mode(UINTN *mode, enum console_mode_change_type how); From ec97e40c294219d8ebaec4c564e01983703e7250 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 09/10] sd-boot: Provide error messages when parsing a config option fails --- src/boot/efi/boot.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 96099e5739..638070d77a 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -995,6 +995,7 @@ static VOID config_defaults_load_from_file(Config *config, CHAR8 *content) { CHAR8 *line; UINTN pos = 0; CHAR8 *key, *value; + EFI_STATUS err; assert(config); assert(content); @@ -1017,32 +1018,23 @@ static VOID config_defaults_load_from_file(Config *config, CHAR8 *content) { } if (strcmpa((CHAR8 *)"editor", key) == 0) { - BOOLEAN on; - - if (EFI_ERROR(parse_boolean(value, &on))) - continue; - - config->editor = on; + err = parse_boolean(value, &config->editor); + if (EFI_ERROR(err)) + log_error_stall(L"Error parsing 'editor' config option: %a", value); continue; } if (strcmpa((CHAR8 *)"auto-entries", key) == 0) { - BOOLEAN on; - - if (EFI_ERROR(parse_boolean(value, &on))) - continue; - - config->auto_entries = on; + err = parse_boolean(value, &config->auto_entries); + if (EFI_ERROR(err)) + log_error_stall(L"Error parsing 'auto-entries' config option: %a", value); continue; } if (strcmpa((CHAR8 *)"auto-firmware", key) == 0) { - BOOLEAN on; - - if (EFI_ERROR(parse_boolean(value, &on))) - continue; - - config->auto_firmware = on; + err = parse_boolean(value, &config->auto_firmware); + if (EFI_ERROR(err)) + log_error_stall(L"Error parsing 'auto-firmware' config option: %a", value); continue; } @@ -1074,8 +1066,11 @@ static VOID config_defaults_load_from_file(Config *config, CHAR8 *content) { else { BOOLEAN on; - if (EFI_ERROR(parse_boolean(value, &on))) + err = parse_boolean(value, &on); + if (EFI_ERROR(err)) { + log_error_stall(L"Error parsing 'random-seed-mode' config option: %a", value); continue; + } config->random_seed_mode = on ? RANDOM_SEED_ALWAYS : RANDOM_SEED_OFF; } From dba0c9832be08909bde250fec3eb521309c167cc Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 11 Aug 2021 14:59:46 +0200 Subject: [PATCH 10/10] sd-boot: Allow on/off and t/f for booleans too --- man/loader.conf.xml | 4 ++-- src/boot/efi/util.c | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/man/loader.conf.xml b/man/loader.conf.xml index 29315ceb17..4c0355800c 100644 --- a/man/loader.conf.xml +++ b/man/loader.conf.xml @@ -41,8 +41,8 @@ a comment line. Empty and comment lines are ignored. Boolean arguments may be written as - yes/y/true/1 or - no/n/false/0. + yes/y/true/t/on/1 or + no/n/false/f/off/0. diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index b2977546a8..0c28cab246 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -75,7 +75,9 @@ EFI_STATUS parse_boolean(const CHAR8 *v, BOOLEAN *b) { if (strcmpa(v, (CHAR8 *)"1") == 0 || strcmpa(v, (CHAR8 *)"yes") == 0 || strcmpa(v, (CHAR8 *)"y") == 0 || - strcmpa(v, (CHAR8 *)"true") == 0) { + strcmpa(v, (CHAR8 *)"true") == 0 || + strcmpa(v, (CHAR8 *)"t") == 0 || + strcmpa(v, (CHAR8 *)"on") == 0) { *b = TRUE; return EFI_SUCCESS; } @@ -83,7 +85,9 @@ EFI_STATUS parse_boolean(const CHAR8 *v, BOOLEAN *b) { if (strcmpa(v, (CHAR8 *)"0") == 0 || strcmpa(v, (CHAR8 *)"no") == 0 || strcmpa(v, (CHAR8 *)"n") == 0 || - strcmpa(v, (CHAR8 *)"false") == 0) { + strcmpa(v, (CHAR8 *)"false") == 0 || + strcmpa(v, (CHAR8 *)"f") == 0 || + strcmpa(v, (CHAR8 *)"off") == 0) { *b = FALSE; return EFI_SUCCESS; }