From aa23f91ed677e4e3f89687c94b74b1afc600324c Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Tue, 6 Sep 2022 10:13:50 +0200 Subject: [PATCH 1/8] boot: Add mempcpy --- src/boot/efi/efi-string.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/boot/efi/efi-string.h b/src/boot/efi/efi-string.h index c25349c0e1..57899c978b 100644 --- a/src/boot/efi/efi-string.h +++ b/src/boot/efi/efi-string.h @@ -112,6 +112,13 @@ bool parse_number16(const char16_t *s, uint64_t *ret_u, const char16_t **ret_tai # define memcmp __builtin_memcmp # define memcpy __builtin_memcpy # define memset __builtin_memset + +static inline void *mempcpy(void * restrict dest, const void * restrict src, size_t n) { + if (!dest || !src || n == 0) + return dest; + memcpy(dest, src, n); + return (uint8_t *) dest + n; +} #endif /* The actual implementations of builtins with efi_ prefix so we can unit test them. */ From 515581d66ab1068c9da8074166dc20e32cab1dce Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Tue, 6 Sep 2022 10:14:25 +0200 Subject: [PATCH 2/8] boot: Use mempcpy --- src/boot/efi/boot.c | 10 ++-------- src/boot/efi/cpio.c | 15 +++++---------- src/boot/efi/stub.c | 23 +++++++---------------- src/boot/efi/util.c | 9 +++------ 4 files changed, 17 insertions(+), 40 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 9c1d95e672..a4804887ac 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -2416,14 +2416,8 @@ static void config_write_entries_to_variable(Config *config) { p = buffer = xmalloc(sz); - for (UINTN i = 0; i < config->entry_count; i++) { - UINTN l; - - l = strsize16(config->entries[i]->id); - memcpy(p, config->entries[i]->id, l); - - p += l; - } + for (UINTN i = 0; i < config->entry_count; i++) + p = mempcpy(p, config->entries[i]->id, strsize16(config->entries[i]->id)); assert(p == buffer + sz); diff --git a/src/boot/efi/cpio.c b/src/boot/efi/cpio.c index 9ffa935e0f..b21cf53b46 100644 --- a/src/boot/efi/cpio.c +++ b/src/boot/efi/cpio.c @@ -116,8 +116,7 @@ static EFI_STATUS pack_cpio_one( *cpio_buffer = a; a = (char *) *cpio_buffer + *cpio_buffer_size; - memcpy(a, "070701", 6); /* magic ID */ - a += 6; + a = mempcpy(a, "070701", 6); /* magic ID */ a = write_cpio_word(a, (*inode_counter)++); /* inode */ a = write_cpio_word(a, access_mode | 0100000 /* = S_IFREG */); /* mode */ @@ -139,16 +138,14 @@ static EFI_STATUS pack_cpio_one( a = write_cpio_word(a, target_dir_prefix_size + fname_size + 2); /* fname size */ a = write_cpio_word(a, 0); /* "crc" */ - memcpy(a, target_dir_prefix, target_dir_prefix_size); - a += target_dir_prefix_size; + a = mempcpy(a, target_dir_prefix, target_dir_prefix_size); *(a++) = '/'; a = mangle_filename(a, fname); /* Pad to next multiple of 4 */ a = pad4(a, *cpio_buffer); - memcpy(a, contents, contents_size); - a += contents_size; + a = mempcpy(a, contents, contents_size); /* Pad to next multiple of 4 */ a = pad4(a, *cpio_buffer); @@ -198,8 +195,7 @@ static EFI_STATUS pack_cpio_dir( *cpio_buffer = a = xrealloc(*cpio_buffer, *cpio_buffer_size, *cpio_buffer_size + l); a = (char *) *cpio_buffer + *cpio_buffer_size; - memcpy(a, "070701", 6); /* magic ID */ - a += 6; + a = mempcpy(a, "070701", 6); /* magic ID */ a = write_cpio_word(a, (*inode_counter)++); /* inode */ a = write_cpio_word(a, access_mode | 0040000 /* = S_IFDIR */); /* mode */ @@ -215,8 +211,7 @@ static EFI_STATUS pack_cpio_dir( a = write_cpio_word(a, path_size + 1); /* fname size */ a = write_cpio_word(a, 0); /* "crc" */ - memcpy(a, path, path_size + 1); - a += path_size + 1; + a = mempcpy(a, path, path_size + 1); /* Pad to next multiple of 4 */ a = pad4(a, *cpio_buffer); diff --git a/src/boot/efi/stub.c b/src/boot/efi/stub.c index 003ae8c99a..3f6832e0c2 100644 --- a/src/boot/efi/stub.c +++ b/src/boot/efi/stub.c @@ -69,8 +69,7 @@ static EFI_STATUS combine_initrd( /* Order matters, the real initrd must come first, since it might include microcode updates * which the kernel only looks for in the first cpio archive */ - memcpy(p, PHYSICAL_ADDRESS_TO_POINTER(initrd_base), initrd_size); - p += initrd_size; + p = mempcpy(p, PHYSICAL_ADDRESS_TO_POINTER(initrd_base), initrd_size); pad = ALIGN4(initrd_size) - initrd_size; if (pad > 0) { @@ -79,20 +78,12 @@ static EFI_STATUS combine_initrd( } } - if (credential_initrd) { - memcpy(p, credential_initrd, credential_initrd_size); - p += credential_initrd_size; - } - - if (global_credential_initrd) { - memcpy(p, global_credential_initrd, global_credential_initrd_size); - p += global_credential_initrd_size; - } - - if (sysext_initrd) { - memcpy(p, sysext_initrd, sysext_initrd_size); - p += sysext_initrd_size; - } + if (credential_initrd) + p = mempcpy(p, credential_initrd, credential_initrd_size); + if (global_credential_initrd) + p = mempcpy(p, global_credential_initrd, global_credential_initrd_size); + if (sysext_initrd) + p = mempcpy(p, sysext_initrd, sysext_initrd_size); assert((uint8_t*) PHYSICAL_ADDRESS_TO_POINTER(base) + n == p); diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index a41dbaa43e..ce1f981817 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -751,14 +751,11 @@ EFI_STATUS make_file_device_path(EFI_HANDLE device, const char16_t *file, EFI_DE end_node = NextDevicePathNode(end_node); size_t file_size = strsize16(file); - size_t dp_size = ((uint8_t *) end_node - (uint8_t *) dp) + END_DEVICE_PATH_LENGTH; + size_t dp_size = (uint8_t *) end_node - (uint8_t *) dp; /* Make a copy that can also hold a file media device path. */ - *ret_dp = xmalloc(dp_size + file_size + SIZE_OF_FILEPATH_DEVICE_PATH); - memcpy(*ret_dp, dp, dp_size); - - /* Point dp to the end node of the copied device path. */ - dp = (EFI_DEVICE_PATH *) ((uint8_t *) *ret_dp + dp_size - END_DEVICE_PATH_LENGTH); + *ret_dp = xmalloc(dp_size + file_size + SIZE_OF_FILEPATH_DEVICE_PATH + END_DEVICE_PATH_LENGTH); + dp = mempcpy(*ret_dp, dp, dp_size); /* Replace end node with file media device path. */ FILEPATH_DEVICE_PATH *file_dp = (FILEPATH_DEVICE_PATH *) dp; From 71c628d4265251778d9bb08fdf59cebd1b1c0128 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 24 Aug 2022 10:31:25 +0200 Subject: [PATCH 3/8] boot: Fix device path unaligned access --- src/boot/efi/disk.c | 13 ++++---- src/boot/efi/util.c | 12 ++++---- src/boot/efi/xbootldr.c | 67 ++++++++++++++++++++++------------------- 3 files changed, 48 insertions(+), 44 deletions(-) diff --git a/src/boot/efi/disk.c b/src/boot/efi/disk.c index 5f889a365c..524662603c 100644 --- a/src/boot/efi/disk.c +++ b/src/boot/efi/disk.c @@ -25,15 +25,14 @@ EFI_STATUS disk_get_part_uuid(EFI_HANDLE *handle, char16_t uuid[static 37]) { if (DevicePathSubType(dp) != MEDIA_HARDDRIVE_DP) continue; - HARDDRIVE_DEVICE_PATH *hd = (HARDDRIVE_DEVICE_PATH *) dp; - if (hd->SignatureType != SIGNATURE_TYPE_GUID) + /* The HD device path may be misaligned. */ + HARDDRIVE_DEVICE_PATH hd; + memcpy(&hd, dp, MIN(sizeof(hd), (size_t) DevicePathNodeLength(dp))); + + if (hd.SignatureType != SIGNATURE_TYPE_GUID) continue; - /* Use memcpy in case the device path node is misaligned. */ - EFI_GUID sig; - memcpy(&sig, hd->Signature, sizeof(hd->Signature)); - - GuidToString(uuid, &sig); + GuidToString(uuid, (EFI_GUID *) &hd.Signature); return EFI_SUCCESS; } diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index ce1f981817..e17a8e18a4 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -757,12 +757,12 @@ EFI_STATUS make_file_device_path(EFI_HANDLE device, const char16_t *file, EFI_DE *ret_dp = xmalloc(dp_size + file_size + SIZE_OF_FILEPATH_DEVICE_PATH + END_DEVICE_PATH_LENGTH); dp = mempcpy(*ret_dp, dp, dp_size); - /* Replace end node with file media device path. */ - FILEPATH_DEVICE_PATH *file_dp = (FILEPATH_DEVICE_PATH *) dp; - file_dp->Header.Type = MEDIA_DEVICE_PATH; - file_dp->Header.SubType = MEDIA_FILEPATH_DP; - memcpy(&file_dp->PathName, file, file_size); - SetDevicePathNodeLength(&file_dp->Header, SIZE_OF_FILEPATH_DEVICE_PATH + file_size); + /* Replace end node with file media device path. Use memcpy() in case dp is unaligned (if accessed as + * FILEPATH_DEVICE_PATH). */ + dp->Type = MEDIA_DEVICE_PATH; + dp->SubType = MEDIA_FILEPATH_DP; + memcpy((uint8_t *) dp + offsetof(FILEPATH_DEVICE_PATH, PathName), file, file_size); + SetDevicePathNodeLength(dp, offsetof(FILEPATH_DEVICE_PATH, PathName) + file_size); dp = NextDevicePathNode(dp); SetDevicePathEndNode(dp); diff --git a/src/boot/efi/xbootldr.c b/src/boot/efi/xbootldr.c index 3ccf9fa8b1..2433816b4e 100644 --- a/src/boot/efi/xbootldr.c +++ b/src/boot/efi/xbootldr.c @@ -12,33 +12,29 @@ union GptHeaderBuffer { uint8_t space[CONST_ALIGN_TO(sizeof(EFI_PARTITION_TABLE_HEADER), 512)]; }; -static EFI_DEVICE_PATH *path_chop(EFI_DEVICE_PATH *path, EFI_DEVICE_PATH *node) { +static EFI_DEVICE_PATH *path_replace_hd( + const EFI_DEVICE_PATH *path, + const EFI_DEVICE_PATH *node, + const HARDDRIVE_DEVICE_PATH *new_node) { + + /* Create a new device path as a copy of path, while chopping off the remainder starting at the given + * node. If new_node is provided, it is appended at the end of the new path. */ + assert(path); assert(node); - UINTN len = (uint8_t *) node - (uint8_t *) path; - EFI_DEVICE_PATH *chopped = xmalloc(len + END_DEVICE_PATH_LENGTH); + size_t len = (uint8_t *) node - (uint8_t *) path, new_node_len = 0; + if (new_node) + new_node_len = DevicePathNodeLength(&new_node->Header); - memcpy(chopped, path, len); - SetDevicePathEndNode((EFI_DEVICE_PATH *) ((uint8_t *) chopped + len)); + EFI_DEVICE_PATH *ret = xmalloc(len + new_node_len + END_DEVICE_PATH_LENGTH); + EFI_DEVICE_PATH *end = mempcpy(ret, path, len); - return chopped; -} + if (new_node) + end = mempcpy(end, new_node, new_node_len); -static EFI_DEVICE_PATH *path_dup(const EFI_DEVICE_PATH *dp) { - assert(dp); - - const EFI_DEVICE_PATH *node = dp; - size_t size = 0; - while (!IsDevicePathEnd(node)) { - size += DevicePathNodeLength(node); - node = NextDevicePathNode(node); - } - size += DevicePathNodeLength(node); - - EFI_DEVICE_PATH *dup = xmalloc(size); - memcpy(dup, dp, size); - return dup; + SetDevicePathEndNode(end); + return ret; } static bool verify_gpt(union GptHeaderBuffer *gpt_header_buffer, EFI_LBA lba_expected) { @@ -149,13 +145,24 @@ static EFI_STATUS try_gpt( if (end < start) /* Bogus? */ continue; - ret_hd->PartitionNumber = i + 1; - ret_hd->PartitionStart = start; - ret_hd->PartitionSize = end - start + 1; - ret_hd->MBRType = MBR_TYPE_EFI_PARTITION_TABLE_HEADER; - ret_hd->SignatureType = SIGNATURE_TYPE_GUID; + *ret_hd = (HARDDRIVE_DEVICE_PATH) { + .Header = { + .Type = MEDIA_DEVICE_PATH, + .SubType = MEDIA_HARDDRIVE_DP, + }, + .PartitionNumber = i + 1, + .PartitionStart = start, + .PartitionSize = end - start + 1, + .MBRType = MBR_TYPE_EFI_PARTITION_TABLE_HEADER, + .SignatureType = SIGNATURE_TYPE_GUID, + }; memcpy(ret_hd->Signature, &entry->UniquePartitionGUID, sizeof(ret_hd->Signature)); + /* HARDDRIVE_DEVICE_PATH has padding, which at least OVMF does not like. */ + SetDevicePathNodeLength( + &ret_hd->Header, + offsetof(HARDDRIVE_DEVICE_PATH, SignatureType) + sizeof(ret_hd->SignatureType)); + return EFI_SUCCESS; } @@ -192,7 +199,7 @@ static EFI_STATUS find_device(EFI_HANDLE *device, EFI_DEVICE_PATH **ret_device_p /* Chop off the partition part, leaving us with the full path to the disk itself. */ _cleanup_free_ EFI_DEVICE_PATH *disk_path = NULL; - EFI_DEVICE_PATH *p = disk_path = path_chop(partition_path, part_node); + EFI_DEVICE_PATH *p = disk_path = path_replace_hd(partition_path, part_node, NULL); EFI_HANDLE disk_handle; EFI_BLOCK_IO_PROTOCOL *block_io; @@ -214,7 +221,6 @@ static EFI_STATUS find_device(EFI_HANDLE *device, EFI_DEVICE_PATH **ret_device_p /* Try several copies of the GPT header, in case one is corrupted */ EFI_LBA backup_lba = 0; - HARDDRIVE_DEVICE_PATH hd = *((HARDDRIVE_DEVICE_PATH *) part_node); for (UINTN nr = 0; nr < 3; nr++) { EFI_LBA lba; @@ -230,6 +236,7 @@ static EFI_STATUS find_device(EFI_HANDLE *device, EFI_DEVICE_PATH **ret_device_p else continue; + HARDDRIVE_DEVICE_PATH hd; err = try_gpt( block_io, lba, nr == 0 ? &backup_lba : NULL, /* Only get backup LBA location from first GPT header. */ @@ -243,9 +250,7 @@ static EFI_STATUS find_device(EFI_HANDLE *device, EFI_DEVICE_PATH **ret_device_p } /* Patch in the data we found */ - EFI_DEVICE_PATH *xboot_path = path_dup(partition_path); - memcpy((uint8_t *) xboot_path + ((uint8_t *) part_node - (uint8_t *) partition_path), &hd, sizeof(hd)); - *ret_device_path = xboot_path; + *ret_device_path = path_replace_hd(partition_path, part_node, &hd); return EFI_SUCCESS; } From 0cf5f816f22c78740e122dfb6b3942ba4241717b Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Mon, 29 Aug 2022 10:09:01 +0200 Subject: [PATCH 4/8] boot: Refuse GPT with invalid entry size SizeOfPartitionEntry must be a multiple of 128 * 2^n. --- src/boot/efi/xbootldr.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/boot/efi/xbootldr.c b/src/boot/efi/xbootldr.c index 2433816b4e..e5b9ca7268 100644 --- a/src/boot/efi/xbootldr.c +++ b/src/boot/efi/xbootldr.c @@ -67,7 +67,7 @@ static bool verify_gpt(union GptHeaderBuffer *gpt_header_buffer, EFI_LBA lba_exp if (h->MyLBA != lba_expected) return false; - if (h->SizeOfPartitionEntry < sizeof(EFI_PARTITION_ENTRY)) + if ((h->SizeOfPartitionEntry % sizeof(EFI_PARTITION_ENTRY)) != 0) return false; if (h->NumberOfPartitionEntries <= 0 || h->NumberOfPartitionEntries > 1024) @@ -130,19 +130,13 @@ static EFI_STATUS try_gpt( /* Now we can finally look for xbootloader partitions. */ for (UINTN i = 0; i < gpt.gpt_header.NumberOfPartitionEntries; i++) { - EFI_PARTITION_ENTRY *entry; - EFI_LBA start, end; - - entry = (EFI_PARTITION_ENTRY*) ((uint8_t*) entries + gpt.gpt_header.SizeOfPartitionEntry * i); + EFI_PARTITION_ENTRY *entry = + (EFI_PARTITION_ENTRY *) ((uint8_t *) entries + gpt.gpt_header.SizeOfPartitionEntry * i); if (memcmp(&entry->PartitionTypeGUID, XBOOTLDR_GUID, sizeof(entry->PartitionTypeGUID)) != 0) continue; - /* Let's use memcpy(), in case the structs are not aligned (they really should be though) */ - memcpy(&start, &entry->StartingLBA, sizeof(start)); - memcpy(&end, &entry->EndingLBA, sizeof(end)); - - if (end < start) /* Bogus? */ + if (entry->EndingLBA < entry->StartingLBA) /* Bogus? */ continue; *ret_hd = (HARDDRIVE_DEVICE_PATH) { @@ -151,8 +145,8 @@ static EFI_STATUS try_gpt( .SubType = MEDIA_HARDDRIVE_DP, }, .PartitionNumber = i + 1, - .PartitionStart = start, - .PartitionSize = end - start + 1, + .PartitionStart = entry->StartingLBA, + .PartitionSize = entry->EndingLBA - entry->StartingLBA + 1, .MBRType = MBR_TYPE_EFI_PARTITION_TABLE_HEADER, .SignatureType = SIGNATURE_TYPE_GUID, }; From d17d0e67707c8630750eca07f175911543fabffb Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 24 Aug 2022 11:01:02 +0200 Subject: [PATCH 5/8] boot: Correctly handle shift keys --- src/boot/efi/console.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/boot/efi/console.c b/src/boot/efi/console.c index f61199ccfd..14c0008afb 100644 --- a/src/boot/efi/console.c +++ b/src/boot/efi/console.c @@ -132,14 +132,19 @@ EFI_STATUS console_key_read(uint64_t *key, uint64_t timeout_usec) { if (FLAGS_SET(keydata.KeyState.KeyShiftState, EFI_SHIFT_STATE_VALID)) { /* Do not distinguish between left and right keys (set both flags). */ - if (keydata.KeyState.KeyShiftState & EFI_SHIFT_PRESSED) - shift |= EFI_SHIFT_PRESSED; if (keydata.KeyState.KeyShiftState & EFI_CONTROL_PRESSED) shift |= EFI_CONTROL_PRESSED; if (keydata.KeyState.KeyShiftState & EFI_ALT_PRESSED) shift |= EFI_ALT_PRESSED; if (keydata.KeyState.KeyShiftState & EFI_LOGO_PRESSED) shift |= EFI_LOGO_PRESSED; + + /* Shift is not supposed to be reported for keys that can be represented as uppercase + * unicode chars (Shift+f is reported as F instead). Some firmware does it anyway, so + * filter those out. */ + if ((keydata.KeyState.KeyShiftState & EFI_SHIFT_PRESSED) && + keydata.Key.UnicodeChar == 0) + shift |= EFI_SHIFT_PRESSED; } /* 32 bit modifier keys + 16 bit scan code + 16 bit unicode */ From 230f78206a579c624f4bf5c5b28c4e70ff14a9d3 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Mon, 29 Aug 2022 10:39:49 +0200 Subject: [PATCH 6/8] boot: Accept Ctrl+Del for deleting words --- man/systemd-boot.xml | 4 +++- src/boot/efi/boot.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/man/systemd-boot.xml b/man/systemd-boot.xml index 99a749b327..7a2d3ec826 100644 --- a/man/systemd-boot.xml +++ b/man/systemd-boot.xml @@ -257,12 +257,13 @@ Esc + Ctrlc Abort the edit and quit the editor Ctrlk - Clear the command line + Clear the command line forwards @@ -272,6 +273,7 @@ + CtrlDel Altd Delete word forwards diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index a4804887ac..c41d40281c 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -254,6 +254,7 @@ static bool line_edit( cursor_left(&cursor, &first); continue; + case KEYPRESS(EFI_CONTROL_PRESSED, SCAN_DELETE, 0): case KEYPRESS(EFI_ALT_PRESSED, 0, 'd'): /* kill-word */ clear = 0; From 28fda21c85d476defbf36a866e2d45729f790eba Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Mon, 29 Aug 2022 10:43:27 +0200 Subject: [PATCH 7/8] boot: Use proper scan codes This arg expects scan codes and it can be very confusing to find a key conflict when trying to add a F3 button when there are no F3 keycodes seemingly in use. CHAR_CARRIAGE_RETURN and SCAN_F3 use the same value, so no changes in behavior. --- src/boot/efi/boot.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index c41d40281c..ac08650c29 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -318,8 +318,8 @@ static bool line_edit( case KEYPRESS(0, 0, CHAR_LINEFEED): case KEYPRESS(0, 0, CHAR_CARRIAGE_RETURN): - case KEYPRESS(0, CHAR_CARRIAGE_RETURN, 0): /* EZpad Mini 4s firmware sends malformed events */ - case KEYPRESS(0, CHAR_CARRIAGE_RETURN, CHAR_CARRIAGE_RETURN): /* Teclast X98+ II firmware sends malformed events */ + case KEYPRESS(0, SCAN_F3, 0): /* EZpad Mini 4s firmware sends malformed events */ + case KEYPRESS(0, SCAN_F3, CHAR_CARRIAGE_RETURN): /* Teclast X98+ II firmware sends malformed events */ if (!streq16(line, *line_in)) { free(*line_in); *line_in = TAKE_PTR(line); @@ -869,8 +869,8 @@ static bool menu_run( case KEYPRESS(0, 0, CHAR_LINEFEED): case KEYPRESS(0, 0, CHAR_CARRIAGE_RETURN): - case KEYPRESS(0, CHAR_CARRIAGE_RETURN, 0): /* EZpad Mini 4s firmware sends malformed events */ - case KEYPRESS(0, CHAR_CARRIAGE_RETURN, CHAR_CARRIAGE_RETURN): /* Teclast X98+ II firmware sends malformed events */ + case KEYPRESS(0, SCAN_F3, 0): /* EZpad Mini 4s firmware sends malformed events */ + case KEYPRESS(0, SCAN_F3, CHAR_CARRIAGE_RETURN): /* Teclast X98+ II firmware sends malformed events */ case KEYPRESS(0, SCAN_RIGHT, 0): exit = true; break; From 97f077df052c75224dcc73375bfaaa69af6a1c26 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Tue, 6 Sep 2022 10:46:27 +0200 Subject: [PATCH 8/8] boot: Avoid magic values in timeout EFI vars --- docs/BOOT_LOADER_INTERFACE.md | 6 +++- src/boot/efi/boot.c | 55 +++++++++++++++++++++++++++++------ 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/docs/BOOT_LOADER_INTERFACE.md b/docs/BOOT_LOADER_INTERFACE.md index f7be50421c..fc9336085b 100644 --- a/docs/BOOT_LOADER_INTERFACE.md +++ b/docs/BOOT_LOADER_INTERFACE.md @@ -31,7 +31,11 @@ variables. All EFI variables use the vendor UUID * The EFI variable `LoaderConfigTimeout` contains the boot menu timeout currently in use. It may be modified both by the boot loader and by the host. The value should be formatted as numeric, NUL-terminated, decimal - string, in UTF-16. The time is specified in µs. + string, in UTF-16. The time is specified in seconds. A value of `menu-force` + will disable the timeout and show the menu indefinitely. If set to `0` or + `menu-hidden` the default entry is booted immediately without showing a menu. + The boot loader should provide a way to interrupt this by for example + listening for key presses for a brief moment before booting. * Similarly, the EFI variable `LoaderConfigTimeoutOneShot` contains a boot menu timeout for a single following boot. It is set by the OS in order to request diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index ac08650c29..a7ef57cb99 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1037,11 +1037,20 @@ static bool menu_run( } if (timeout_efivar_saved != config->timeout_sec_efivar) { - if (config->timeout_sec_efivar == TIMEOUT_UNSET) + switch (config->timeout_sec_efivar) { + case TIMEOUT_UNSET: efivar_set(LOADER_GUID, L"LoaderConfigTimeout", NULL, EFI_VARIABLE_NON_VOLATILE); - else + break; + case TIMEOUT_MENU_FORCE: + efivar_set(LOADER_GUID, u"LoaderConfigTimeout", u"menu-force", EFI_VARIABLE_NON_VOLATILE); + break; + case TIMEOUT_MENU_HIDDEN: + efivar_set(LOADER_GUID, u"LoaderConfigTimeout", u"menu-hidden", EFI_VARIABLE_NON_VOLATILE); + break; + default: efivar_set_uint_string(LOADER_GUID, L"LoaderConfigTimeout", config->timeout_sec_efivar, EFI_VARIABLE_NON_VOLATILE); + } } clear_screen(COLOR_NORMAL); @@ -1532,6 +1541,34 @@ static void config_entry_add_type1( TAKE_PTR(entry); } +static EFI_STATUS efivar_get_timeout(const char16_t *var, uint32_t *ret_value) { + _cleanup_free_ char16_t *value = NULL; + EFI_STATUS err; + + assert(var); + assert(ret_value); + + err = efivar_get(LOADER_GUID, var, &value); + if (err != EFI_SUCCESS) + return err; + + if (streq16(value, u"menu-force")) { + *ret_value = TIMEOUT_MENU_FORCE; + return EFI_SUCCESS; + } + if (streq16(value, u"menu-hidden")) { + *ret_value = TIMEOUT_MENU_HIDDEN; + return EFI_SUCCESS; + } + + uint64_t timeout; + if (!parse_number16(value, &timeout, NULL)) + return EFI_INVALID_PARAMETER; + + *ret_value = MIN(timeout, TIMEOUT_TYPE_MAX); + return EFI_SUCCESS; +} + static void config_load_defaults(Config *config, EFI_FILE *root_dir) { _cleanup_free_ char *content = NULL; UINTN value; @@ -1557,20 +1594,20 @@ static void config_load_defaults(Config *config, EFI_FILE *root_dir) { if (err == EFI_SUCCESS) config_defaults_load_from_file(config, content); - err = efivar_get_uint_string(LOADER_GUID, L"LoaderConfigTimeout", &value); - if (err == EFI_SUCCESS) { - config->timeout_sec_efivar = MIN(value, TIMEOUT_TYPE_MAX); + err = efivar_get_timeout(u"LoaderConfigTimeout", &config->timeout_sec_efivar); + if (err == EFI_SUCCESS) config->timeout_sec = config->timeout_sec_efivar; - } + else if (err != EFI_NOT_FOUND) + log_error_stall(u"Error reading LoaderConfigTimeout EFI variable: %r", err); - err = efivar_get_uint_string(LOADER_GUID, L"LoaderConfigTimeoutOneShot", &value); + 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(LOADER_GUID, L"LoaderConfigTimeoutOneShot", NULL, EFI_VARIABLE_NON_VOLATILE); - config->timeout_sec = MIN(value, TIMEOUT_TYPE_MAX); config->force_menu = true; /* force the menu when this is set */ - } + } else if (err != EFI_NOT_FOUND) + log_error_stall(u"Error reading LoaderConfigTimeoutOneShot EFI variable: %r", err); err = efivar_get_uint_string(LOADER_GUID, L"LoaderConfigConsoleMode", &value); if (err == EFI_SUCCESS)