From 596731db99338876710d3275243cd15e52cdff83 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 28 Jun 2024 19:43:31 +0200 Subject: [PATCH 01/12] efi: add limit on how large files can be we load into memory at once --- src/boot/efi/util.c | 14 ++++++++++---- src/boot/efi/util.h | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index 6ceb032fa1..3639afcb15 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -9,6 +9,9 @@ #include "version.h" #include "efivars.h" +/* Never try to read more than 16G into memory (and on 32bit 1G) */ +#define FILE_READ_MAX MIN(SIZE_MAX/4, UINT64_C(16)*1024U*1024U*1024U) + void convert_efi_path(char16_t *path) { assert(path); @@ -107,7 +110,7 @@ EFI_STATUS chunked_read(EFI_FILE *file, size_t *size, void *buf) { EFI_STATUS file_read( EFI_FILE *dir, const char16_t *name, - uint64_t off, + uint64_t offset, size_t size, char **ret, size_t *ret_size) { @@ -131,14 +134,17 @@ EFI_STATUS file_read( if (err != EFI_SUCCESS) return err; - if (info->FileSize > SIZE_MAX) + if (info->FileSize > SIZE_MAX) /* overflow check */ return EFI_BAD_BUFFER_SIZE; size = info->FileSize; } - if (off > 0) { - err = handle->SetPosition(handle, off); + if (size > FILE_READ_MAX) /* make sure we don't read unbounded data into RAM */ + return EFI_BAD_BUFFER_SIZE; + + if (offset > 0) { + err = handle->SetPosition(handle, offset); if (err != EFI_SUCCESS) return err; } diff --git a/src/boot/efi/util.h b/src/boot/efi/util.h index 3a26bc5c92..dc16f3fb76 100644 --- a/src/boot/efi/util.h +++ b/src/boot/efi/util.h @@ -86,7 +86,7 @@ char16_t *xstr8_to_path(const char *stra); char16_t *mangle_stub_cmdline(char16_t *cmdline); EFI_STATUS chunked_read(EFI_FILE *file, size_t *size, void *buf); -EFI_STATUS file_read(EFI_FILE *dir, const char16_t *name, uint64_t off, size_t size, char **content, size_t *content_size); +EFI_STATUS file_read(EFI_FILE *dir, const char16_t *name, uint64_t offset, size_t size, char **content, size_t *content_size); static inline void file_closep(EFI_FILE **handle) { if (!*handle) From d30b89c7fc46d87774fd6eb6770c1e024b97395c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 2 Jul 2024 16:01:28 +0200 Subject: [PATCH 02/12] efi: fix mangle_stub_cmdline() for empty strings --- src/boot/efi/util.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index 3639afcb15..aafaf06e98 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -42,19 +42,17 @@ static bool shall_be_whitespace(char16_t c) { } char16_t* mangle_stub_cmdline(char16_t *cmdline) { - char16_t *p, *q, *e; - if (!cmdline) return cmdline; - p = q = cmdline; - /* Skip initial whitespace */ - while (shall_be_whitespace(*p)) + const char16_t *p = cmdline; + while (*p != 0 && shall_be_whitespace(*p)) p++; /* Turn inner control characters into proper spaces */ - for (e = p; *p != 0; p++) { + char16_t *e = cmdline; + for (char16_t *q = cmdline; *p != 0; p++) { if (shall_be_whitespace(*p)) { *(q++) = ' '; continue; From 1beaaeab2fde2ae4d1a948e661241ca11096167e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 1 Jul 2024 17:01:26 +0200 Subject: [PATCH 03/12] efi: drop "ret_" prefix from "ret_sections[]" parameter While we write data to this parameter, it's not really a return parameter, we after all do not fully set it, we just fill in some fields. Hence it must be initialized beforehand. According to our coding style only parameters that are purely used for returning something should be named "ret_xyz", hence this one should not be. (We'll later rely on the current behaviour that it leaves array entries for which we find no sections untouched, hence leave behaviour as is, just rename the parameters to something more appropriate). (Since we are dropping the "ret_" prefix of "ret_sections", let's rename the old "section" parameter at the same time to "section_names", to make clearer what it is about). --- src/boot/efi/pe.c | 42 ++++++++++++++++++++++-------------------- src/boot/efi/pe.h | 8 ++++---- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/src/boot/efi/pe.c b/src/boot/efi/pe.c index 587e3ef7b1..33aeaeab50 100644 --- a/src/boot/efi/pe.c +++ b/src/boot/efi/pe.c @@ -178,22 +178,22 @@ static bool pe_section_name_equal(const char *a, const char *b) { static void pe_locate_sections( const PeSectionHeader section_table[], size_t n_section_table, - const char * const sections[], + const char *const section_names[], size_t validate_base, - PeSectionVector *ret_sections) { + PeSectionVector sections[]) { assert(section_table || n_section_table == 0); + assert(section_names); assert(sections); - assert(ret_sections); /* Searches for the sections listed in 'sections[]' within the section table. Validates the resulted * data. If 'validate_base' is non-zero also takes base offset when loaded into memory into account for * qchecking for overflows. */ - for (size_t i = 0; sections[i]; i++) + for (size_t i = 0; section_names[i]; i++) FOREACH_ARRAY(j, section_table, n_section_table) { - if (!pe_section_name_equal((const char*) j->Name, sections[i])) + if (!pe_section_name_equal((const char*) j->Name, section_names[i])) continue; /* Overflow check: ignore sections that are impossibly large, relative to the file @@ -220,7 +220,7 @@ static void pe_locate_sections( } /* At this time, the sizes and offsets have been validated. Store them away */ - ret_sections[i] = (PeSectionVector) { + sections[i] = (PeSectionVector) { .size = j->VirtualSize, .file_offset = j->PointerToRawData, .memory_offset = j->VirtualAddress, @@ -232,17 +232,19 @@ static void pe_locate_sections( } static uint32_t get_compatibility_entry_address(const DosFileHeader *dos, const PeFileHeader *pe) { - static const char *sections[] = { ".compat", NULL }; - PeSectionVector vector = {}; - /* The kernel may provide alternative PE entry points for different PE architectures. This allows * booting a 64-bit kernel on 32-bit EFI that is otherwise running on a 64-bit CPU. The locations of any * such compat entry points are located in a special PE section. */ + assert(dos); + assert(pe); + + static const char *const section_names[] = { ".compat", NULL }; + PeSectionVector vector = {}; pe_locate_sections( (const PeSectionHeader *) ((const uint8_t *) dos + section_table_offset(dos, pe)), pe->FileHeader.NumberOfSections, - sections, + section_names, PTR_TO_SIZE(dos), &vector); @@ -308,16 +310,16 @@ EFI_STATUS pe_kernel_info(const void *base, uint32_t *ret_compat_address) { EFI_STATUS pe_memory_locate_sections( const void *base, - const char* const sections[], - PeSectionVector *ret_sections) { + const char *const section_names[], + PeSectionVector sections[]) { const DosFileHeader *dos; const PeFileHeader *pe; size_t offset; assert(base); + assert(section_names); assert(sections); - assert(ret_sections); dos = (const DosFileHeader *) base; if (!verify_dos(dos)) @@ -331,9 +333,9 @@ EFI_STATUS pe_memory_locate_sections( pe_locate_sections( (const PeSectionHeader *) ((const uint8_t *) base + offset), pe->FileHeader.NumberOfSections, - sections, + section_names, PTR_TO_SIZE(base), - ret_sections); + sections); return EFI_SUCCESS; } @@ -341,8 +343,8 @@ EFI_STATUS pe_memory_locate_sections( EFI_STATUS pe_file_locate_sections( EFI_FILE *dir, const char16_t *path, - const char * const sections[], - PeSectionVector *ret_sections) { + const char *const section_names[], + PeSectionVector sections[]) { _cleanup_free_ PeSectionHeader *section_table = NULL; _cleanup_(file_closep) EFI_FILE *handle = NULL; DosFileHeader dos; @@ -352,8 +354,8 @@ EFI_STATUS pe_file_locate_sections( assert(dir); assert(path); + assert(section_names); assert(sections); - assert(ret_sections); err = dir->Open(dir, &handle, (char16_t *) path, EFI_FILE_MODE_READ, 0ULL); if (err != EFI_SUCCESS) @@ -402,9 +404,9 @@ EFI_STATUS pe_file_locate_sections( pe_locate_sections( section_table, pe.FileHeader.NumberOfSections, - sections, + section_names, /* validate_base= */ 0, /* don't validate base */ - ret_sections); + sections); return EFI_SUCCESS; } diff --git a/src/boot/efi/pe.h b/src/boot/efi/pe.h index bc6d74beeb..e55ac5daa9 100644 --- a/src/boot/efi/pe.h +++ b/src/boot/efi/pe.h @@ -17,13 +17,13 @@ static inline bool PE_SECTION_VECTOR_IS_SET(const PeSectionVector *v) { EFI_STATUS pe_memory_locate_sections( const void *base, - const char * const sections[], - PeSectionVector *ret_sections); + const char *const section_names[], + PeSectionVector sections[]); EFI_STATUS pe_file_locate_sections( EFI_FILE *dir, const char16_t *path, - const char * const sections[], - PeSectionVector *ret_sections); + const char *const section_names[], + PeSectionVector sections[]); EFI_STATUS pe_kernel_info(const void *base, uint32_t *ret_compat_address); From 6c741d8f500cfeab0b6de69ca46c8e09f92ccf53 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 1 Jul 2024 17:39:49 +0200 Subject: [PATCH 04/12] efi: don't non-chalantly drop const from memory buffer --- src/boot/efi/pe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/boot/efi/pe.c b/src/boot/efi/pe.c index 33aeaeab50..587960e321 100644 --- a/src/boot/efi/pe.c +++ b/src/boot/efi/pe.c @@ -261,7 +261,7 @@ static uint32_t get_compatibility_entry_address(const DosFileHeader *dos, const size_t addr = vector.memory_offset, size = vector.size; while (size >= sizeof(LinuxPeCompat1) && addr % alignof(LinuxPeCompat1) == 0) { - LinuxPeCompat1 *compat = (LinuxPeCompat1 *) ((uint8_t *) dos + addr); + const LinuxPeCompat1 *compat = (const LinuxPeCompat1 *) ((const uint8_t *) dos + addr); if (compat->type == 0 || compat->size == 0 || compat->size > size) break; From b8ed6e22f01084530da8918a7da50c7327fa1987 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 28 Jun 2024 19:47:46 +0200 Subject: [PATCH 05/12] boot: indent error code path, but leave main code path unindented --- src/boot/efi/boot.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index b3d9329926..aca34a5145 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1664,9 +1664,16 @@ static void config_load_type1_entries( if (startswith(f->FileName, u"auto-")) continue; - err = file_read(entries_dir, f->FileName, 0, 0, &content, NULL); - if (err == EFI_SUCCESS) - boot_entry_add_type1(config, device, root_dir, u"\\loader\\entries", f->FileName, content, loaded_image_path); + err = file_read(entries_dir, + f->FileName, + /* offset= */ 0, + /* size= */ 0, + &content, + /* ret_size= */ NULL); + if (err != EFI_SUCCESS) + continue; + + boot_entry_add_type1(config, device, root_dir, u"\\loader\\entries", f->FileName, content, loaded_image_path); } } From 76c8c0b2647c8aff23b9ecbd5b93637994688666 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 28 Jun 2024 19:56:55 +0200 Subject: [PATCH 06/12] boot: split out inner part of config_load_type2_entries() Let's simplify the code a bit, and parse Type 2 entries in a function of its own, separate from the directory enumeration. This closely follows a similar split we did a long time ago for Type 1. This is just refactoring, no real code change. --- src/boot/efi/boot.c | 253 +++++++++++++++++++++++--------------------- 1 file changed, 134 insertions(+), 119 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index aca34a5145..1d31f9097f 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -2088,6 +2088,139 @@ static void config_add_entry_windows(Config *config, EFI_HANDLE *device, EFI_FIL #endif } +static void boot_entry_add_type2( + Config *config, + EFI_HANDLE *device, + EFI_FILE *dir, + const uint16_t *filename) { + + enum { + SECTION_CMDLINE, + SECTION_OSREL, + _SECTION_MAX, + }; + static const char * const section_names[_SECTION_MAX + 1] = { + [SECTION_CMDLINE] = ".cmdline", + [SECTION_OSREL] = ".osrel", + NULL, + }; + + EFI_STATUS err; + + assert(config); + assert(device); + assert(dir); + assert(filename); + + /* Look for .osrel and .cmdline sections in the .efi binary */ + PeSectionVector sections[_SECTION_MAX] = {}; + err = pe_file_locate_sections(dir, filename, section_names, sections); + if (err != EFI_SUCCESS || !PE_SECTION_VECTOR_IS_SET(sections + SECTION_OSREL)) + return; + + _cleanup_free_ char *content = NULL; + err = file_read(dir, + filename, + sections[SECTION_OSREL].file_offset, + sections[SECTION_OSREL].size, + &content, + /* ret_size= */ NULL); + if (err != EFI_SUCCESS) + return; + + _cleanup_free_ char16_t *os_pretty_name = NULL, *os_image_id = NULL, *os_name = NULL, *os_id = NULL, + *os_image_version = NULL, *os_version = NULL, *os_version_id = NULL, *os_build_id = NULL; + char *line, *key, *value; + size_t pos = 0; + + /* read properties from the embedded os-release file */ + while ((line = line_get_key_value(content, "=", &pos, &key, &value))) + if (streq8(key, "PRETTY_NAME")) { + free(os_pretty_name); + os_pretty_name = xstr8_to_16(value); + + } else if (streq8(key, "IMAGE_ID")) { + free(os_image_id); + os_image_id = xstr8_to_16(value); + + } else if (streq8(key, "NAME")) { + free(os_name); + os_name = xstr8_to_16(value); + + } else if (streq8(key, "ID")) { + free(os_id); + os_id = xstr8_to_16(value); + + } else if (streq8(key, "IMAGE_VERSION")) { + free(os_image_version); + os_image_version = xstr8_to_16(value); + + } else if (streq8(key, "VERSION")) { + free(os_version); + os_version = xstr8_to_16(value); + + } else if (streq8(key, "VERSION_ID")) { + free(os_version_id); + os_version_id = xstr8_to_16(value); + + } else if (streq8(key, "BUILD_ID")) { + free(os_build_id); + os_build_id = xstr8_to_16(value); + } + + const char16_t *good_name, *good_version, *good_sort_key; + if (!bootspec_pick_name_version_sort_key( + os_pretty_name, + os_image_id, + os_name, + os_id, + os_image_version, + os_version, + os_version_id, + os_build_id, + &good_name, + &good_version, + &good_sort_key)) + return; + + BootEntry *entry = xnew(BootEntry, 1); + *entry = (BootEntry) { + .id = xstrdup16(filename), + .type = LOADER_UNIFIED_LINUX, + .title = xstrdup16(good_name), + .version = xstrdup16(good_version), + .device = device, + .loader = xasprintf("\\EFI\\Linux\\%ls", filename), + .sort_key = xstrdup16(good_sort_key), + .key = 'l', + .tries_done = -1, + .tries_left = -1, + }; + + strtolower16(entry->id); + config_add_entry(config, entry); + boot_entry_parse_tries(entry, u"\\EFI\\Linux", filename, u".efi"); + + if (!PE_SECTION_VECTOR_IS_SET(sections + SECTION_CMDLINE)) + return; + + content = mfree(content); + + /* read the embedded cmdline file */ + size_t cmdline_len; + err = file_read(dir, + filename, + sections[SECTION_CMDLINE].file_offset, + sections[SECTION_CMDLINE].size, + &content, + &cmdline_len); + if (err == EFI_SUCCESS) { + entry->options = xstrn8_to_16(content, cmdline_len); + mangle_stub_cmdline(entry->options); + entry->options_implied = true; + } +} + static void config_load_type2_entries( Config *config, EFI_HANDLE *device, @@ -2109,26 +2242,6 @@ static void config_load_type2_entries( return; for (;;) { - enum { - SECTION_CMDLINE, - SECTION_OSREL, - _SECTION_MAX, - }; - - static const char * const section_names[_SECTION_MAX + 1] = { - [SECTION_CMDLINE] = ".cmdline", - [SECTION_OSREL] = ".osrel", - NULL, - }; - - _cleanup_free_ char16_t *os_pretty_name = NULL, *os_image_id = NULL, *os_name = NULL, *os_id = NULL, - *os_image_version = NULL, *os_version = NULL, *os_version_id = NULL, *os_build_id = NULL; - const char16_t *good_name, *good_version, *good_sort_key; - _cleanup_free_ char *content = NULL; - PeSectionVector sections[_SECTION_MAX] = {}; - char *line, *key, *value; - size_t pos = 0; - err = readdir(linux_dir, &f, &f_size); if (err != EFI_SUCCESS || !f) break; @@ -2142,105 +2255,7 @@ static void config_load_type2_entries( if (startswith(f->FileName, u"auto-")) continue; - /* look for .osrel and .cmdline sections in the .efi binary */ - err = pe_file_locate_sections(linux_dir, f->FileName, section_names, sections); - if (err != EFI_SUCCESS || !PE_SECTION_VECTOR_IS_SET(sections + SECTION_OSREL)) - continue; - - err = file_read(linux_dir, - f->FileName, - sections[SECTION_OSREL].file_offset, - sections[SECTION_OSREL].size, - &content, - NULL); - if (err != EFI_SUCCESS) - continue; - - /* read properties from the embedded os-release file */ - while ((line = line_get_key_value(content, "=", &pos, &key, &value))) - if (streq8(key, "PRETTY_NAME")) { - free(os_pretty_name); - os_pretty_name = xstr8_to_16(value); - - } else if (streq8(key, "IMAGE_ID")) { - free(os_image_id); - os_image_id = xstr8_to_16(value); - - } else if (streq8(key, "NAME")) { - free(os_name); - os_name = xstr8_to_16(value); - - } else if (streq8(key, "ID")) { - free(os_id); - os_id = xstr8_to_16(value); - - } else if (streq8(key, "IMAGE_VERSION")) { - free(os_image_version); - os_image_version = xstr8_to_16(value); - - } else if (streq8(key, "VERSION")) { - free(os_version); - os_version = xstr8_to_16(value); - - } else if (streq8(key, "VERSION_ID")) { - free(os_version_id); - os_version_id = xstr8_to_16(value); - - } else if (streq8(key, "BUILD_ID")) { - free(os_build_id); - os_build_id = xstr8_to_16(value); - } - - if (!bootspec_pick_name_version_sort_key( - os_pretty_name, - os_image_id, - os_name, - os_id, - os_image_version, - os_version, - os_version_id, - os_build_id, - &good_name, - &good_version, - &good_sort_key)) - continue; - - BootEntry *entry = xnew(BootEntry, 1); - *entry = (BootEntry) { - .id = xstrdup16(f->FileName), - .type = LOADER_UNIFIED_LINUX, - .title = xstrdup16(good_name), - .version = xstrdup16(good_version), - .device = device, - .loader = xasprintf("\\EFI\\Linux\\%ls", f->FileName), - .sort_key = xstrdup16(good_sort_key), - .key = 'l', - .tries_done = -1, - .tries_left = -1, - }; - - strtolower16(entry->id); - config_add_entry(config, entry); - boot_entry_parse_tries(entry, u"\\EFI\\Linux", f->FileName, u".efi"); - - if (!PE_SECTION_VECTOR_IS_SET(sections + SECTION_CMDLINE)) - continue; - - content = mfree(content); - - /* read the embedded cmdline file */ - size_t cmdline_len; - err = file_read(linux_dir, - f->FileName, - sections[SECTION_CMDLINE].file_offset, - sections[SECTION_CMDLINE].size, - &content, - &cmdline_len); - if (err == EFI_SUCCESS) { - entry->options = xstrn8_to_16(content, cmdline_len); - mangle_stub_cmdline(entry->options); - entry->options_implied = true; - } + boot_entry_add_type2(config, device, linux_dir, f->FileName); } } From 759cf7e10ae774342835dcb962e6b1144e9bc1a4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 2 Jul 2024 16:00:30 +0200 Subject: [PATCH 07/12] boot: compare auto- prefix case-insensitively --- src/boot/efi/boot.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 1d31f9097f..ff05e51c4c 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1658,10 +1658,9 @@ static void config_load_type1_entries( continue; if (FLAGS_SET(f->Attribute, EFI_FILE_DIRECTORY)) continue; - if (!endswith_no_case(f->FileName, u".conf")) continue; - if (startswith(f->FileName, u"auto-")) + if (startswith_no_case(f->FileName, u"auto-")) continue; err = file_read(entries_dir, @@ -2252,7 +2251,7 @@ static void config_load_type2_entries( continue; if (!endswith_no_case(f->FileName, u".efi")) continue; - if (startswith(f->FileName, u"auto-")) + if (startswith_no_case(f->FileName, u"auto-")) continue; boot_entry_add_type2(config, device, linux_dir, f->FileName); From c8bcf7ecf715aadd077cb2850e24122bebf19b6e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 2 Jul 2024 09:14:38 +0200 Subject: [PATCH 08/12] measure: drop incomplete support for PCRs != 11 At this point we have a clearer model: * systemd-measure should be used for measuring UKIs on vendor build systems, i.e. only cover stuff predictable by the OS vendor, and identical on all systems. And that is pretty much only PCR 11. * systemd-pcrlock should cover the other PCRs, which carry inherently local information, and can only be predicted locally and not already on vendor build systems. Because of that, let's not bother with any PCRs except for 11 in systemd-measure. This was added at a time where systemd-pcrlock didn't exist yet, and hence it wasn't clear how this will play out in the end. --- man/systemd-measure.xml | 10 +-- src/boot/measure.c | 133 ++++++++++++++++++---------------------- 2 files changed, 63 insertions(+), 80 deletions(-) diff --git a/man/systemd-measure.xml b/man/systemd-measure.xml index 8ea667426e..931b62c12e 100644 --- a/man/systemd-measure.xml +++ b/man/systemd-measure.xml @@ -17,7 +17,7 @@ systemd-measure - Pre-calculate and sign expected TPM2 PCR values for booted unified kernel images + Pre-calculate and sign expected TPM2 PCR 11 values for booted unified kernel images @@ -62,7 +62,7 @@ status This is the default command if none is specified. This queries the local system's - TPM2 PCR 11+12+13 values and displays them. The data is written in a similar format as the + TPM2 PCR 11 values and displays them. The data is written in a similar format as the calculate command below, and may be used to quickly compare expectation with reality. @@ -76,9 +76,9 @@ kernel image consisting of the components specified with , , , , , , , - , , see below. - Only is mandatory. (Alternatively, specify to use the current values of PCR - register 11 instead.) + , , see below. Only + is mandatory. (Alternatively, specify to use the + current values of PCR register 11 instead.) diff --git a/src/boot/measure.c b/src/boot/measure.c index 9fdc37dfb5..81f1a9fbd2 100644 --- a/src/boot/measure.c +++ b/src/boot/measure.c @@ -1017,14 +1017,6 @@ static int validate_stub(void) { if (r < 0) return r; - r = compare_reported_pcr_nr(TPM2_PCR_KERNEL_CONFIG, EFI_LOADER_VARIABLE(StubPcrKernelParameters), "kernel parameters"); - if (r < 0) - return r; - - r = compare_reported_pcr_nr(TPM2_PCR_SYSEXTS, EFI_LOADER_VARIABLE(StubPcrInitRDSysExts), "initrd system extension images"); - if (r < 0) - return r; - STRV_FOREACH(bank, arg_banks) { _cleanup_free_ char *b = NULL, *p = NULL; @@ -1049,12 +1041,6 @@ static int validate_stub(void) { } static int verb_status(int argc, char *argv[], void *userdata) { - static const uint32_t relevant_pcrs[] = { - TPM2_PCR_KERNEL_BOOT, - TPM2_PCR_KERNEL_CONFIG, - TPM2_PCR_SYSEXTS, - }; - _cleanup_(sd_json_variant_unrefp) sd_json_variant *v = NULL; int r; @@ -1062,72 +1048,69 @@ static int verb_status(int argc, char *argv[], void *userdata) { if (r < 0) return r; - for (size_t i = 0; i < ELEMENTSOF(relevant_pcrs); i++) { + STRV_FOREACH(bank, arg_banks) { + _cleanup_free_ char *b = NULL, *p = NULL, *s = NULL; + _cleanup_free_ void *h = NULL; + size_t l; - STRV_FOREACH(bank, arg_banks) { - _cleanup_free_ char *b = NULL, *p = NULL, *s = NULL; - _cleanup_free_ void *h = NULL; - size_t l; + b = strdup(*bank); + if (!b) + return log_oom(); - b = strdup(*bank); - if (!b) + if (asprintf(&p, "/sys/class/tpm/tpm0/pcr-%s/%" PRIu32, ascii_strlower(b), (uint32_t) TPM2_PCR_KERNEL_BOOT) < 0) + return log_oom(); + + r = read_virtual_file(p, 4096, &s, NULL); + if (r == -ENOENT) + continue; + if (r < 0) + return log_error_errno(r, "Failed to read '%s': %m", p); + + r = unhexmem(strstrip(s), &h, &l); + if (r < 0) + return log_error_errno(r, "Failed to decode PCR value '%s': %m", s); + + if (arg_json_format_flags & SD_JSON_FORMAT_OFF) { + _cleanup_free_ char *f = NULL; + + f = hexmem(h, l); + if (!h) return log_oom(); - if (asprintf(&p, "/sys/class/tpm/tpm0/pcr-%s/%" PRIu32, ascii_strlower(b), relevant_pcrs[i]) < 0) - return log_oom(); - - r = read_virtual_file(p, 4096, &s, NULL); - if (r == -ENOENT) - continue; - if (r < 0) - return log_error_errno(r, "Failed to read '%s': %m", p); - - r = unhexmem(strstrip(s), &h, &l); - if (r < 0) - return log_error_errno(r, "Failed to decode PCR value '%s': %m", s); - - if (arg_json_format_flags & SD_JSON_FORMAT_OFF) { - _cleanup_free_ char *f = NULL; - - f = hexmem(h, l); - if (!h) - return log_oom(); - - if (bank == arg_banks) { - /* before the first line for each PCR, write a short descriptive text to - * stderr, and leave the primary content on stdout */ - fflush(stdout); - fprintf(stderr, "%s# PCR[%" PRIu32 "] %s%s%s\n", - ansi_grey(), - relevant_pcrs[i], - tpm2_pcr_index_to_string(relevant_pcrs[i]), - memeqzero(h, l) ? " (NOT SET!)" : "", - ansi_normal()); - fflush(stderr); - } - - printf("%" PRIu32 ":%s=%s\n", relevant_pcrs[i], b, f); - - } else { - _cleanup_(sd_json_variant_unrefp) sd_json_variant *bv = NULL, *a = NULL; - - r = sd_json_buildo( - &bv, - SD_JSON_BUILD_PAIR("pcr", SD_JSON_BUILD_INTEGER(relevant_pcrs[i])), - SD_JSON_BUILD_PAIR("hash", SD_JSON_BUILD_HEX(h, l))); - if (r < 0) - return log_error_errno(r, "Failed to build JSON object: %m"); - - a = sd_json_variant_ref(sd_json_variant_by_key(v, b)); - - r = sd_json_variant_append_array(&a, bv); - if (r < 0) - return log_error_errno(r, "Failed to append PCR entry to JSON array: %m"); - - r = sd_json_variant_set_field(&v, b, a); - if (r < 0) - return log_error_errno(r, "Failed to add bank info to object: %m"); + if (bank == arg_banks) { + /* before the first line for each PCR, write a short descriptive text to + * stderr, and leave the primary content on stdout */ + fflush(stdout); + fprintf(stderr, "%s# PCR[%" PRIu32 "] %s%s%s\n", + ansi_grey(), + (uint32_t) TPM2_PCR_KERNEL_BOOT, + tpm2_pcr_index_to_string(TPM2_PCR_KERNEL_BOOT), + memeqzero(h, l) ? " (NOT SET!)" : "", + ansi_normal()); + fflush(stderr); } + + printf("%" PRIu32 ":%s=%s\n", (uint32_t) TPM2_PCR_KERNEL_BOOT, b, f); + + } else { + _cleanup_(sd_json_variant_unrefp) sd_json_variant *bv = NULL, *a = NULL; + + r = sd_json_buildo( + &bv, + SD_JSON_BUILD_PAIR("pcr", SD_JSON_BUILD_INTEGER(TPM2_PCR_KERNEL_BOOT)), + SD_JSON_BUILD_PAIR("hash", SD_JSON_BUILD_HEX(h, l))); + if (r < 0) + return log_error_errno(r, "Failed to build JSON object: %m"); + + a = sd_json_variant_ref(sd_json_variant_by_key(v, b)); + + r = sd_json_variant_append_array(&a, bv); + if (r < 0) + return log_error_errno(r, "Failed to append PCR entry to JSON array: %m"); + + r = sd_json_variant_set_field(&v, b, a); + if (r < 0) + return log_error_errno(r, "Failed to add bank info to object: %m"); } } From ea22cbc3278b42b0e0d96338e60a135fe37f73f1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 2 Jul 2024 12:35:57 +0200 Subject: [PATCH 09/12] ukify: suffix switches that take parameters with = in log output --- src/ukify/ukify.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index f1db9ba578..76437f25e5 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -1636,7 +1636,7 @@ def finalize_options(opts): elif len(opts.positional) == 1 and opts.positional[0] in VERBS: opts.verb = opts.positional[0] elif opts.linux or opts.initrd: - raise ValueError('--linux/--initrd options cannot be used with positional arguments') + raise ValueError('--linux=/--initrd= options cannot be used with positional arguments') else: print("Assuming obsolete command line syntax with no verb. Please use 'build'.") if opts.positional: From 2fda6f5fffcc05adaa5a08d976e09ad7cc97c1b3 Mon Sep 17 00:00:00 2001 From: Brenton Simpson Date: Wed, 3 Jul 2024 15:40:26 +0200 Subject: [PATCH 10/12] boot: cover for hardware keys on phones/tablets The patch is originally from Brenton Simpson, I (Lennart) just added some comments and rebased it. I didn't test this, but the patch looks so obviously right to me, that I think we should just merge it, instead of delaying this further. In the worst case noone notices, in the best case this makes sd-boot work reasonably nicely on devices that only have a hadware power key + volume rocker. Fixes: #30598 Replaces: #31135 --- src/boot/efi/boot.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index ff05e51c4c..b281136c05 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -881,6 +881,7 @@ static bool menu_run( switch (key) { case KEYPRESS(0, SCAN_UP, 0): + case KEYPRESS(0, SCAN_VOLUME_UP, 0): /* Handle phones/tablets that only have a volume up/down rocker + power key (and otherwise just touchscreen input) */ case KEYPRESS(0, 0, 'k'): case KEYPRESS(0, 0, 'K'): if (idx_highlight > 0) @@ -888,6 +889,7 @@ static bool menu_run( break; case KEYPRESS(0, SCAN_DOWN, 0): + case KEYPRESS(0, SCAN_VOLUME_DOWN, 0): case KEYPRESS(0, 0, 'j'): case KEYPRESS(0, 0, 'J'): if (idx_highlight < config->n_entries-1) @@ -925,9 +927,10 @@ static bool menu_run( case KEYPRESS(0, 0, '\n'): case KEYPRESS(0, 0, '\r'): - case KEYPRESS(0, SCAN_F3, 0): /* EZpad Mini 4s firmware sends malformed events */ - case KEYPRESS(0, SCAN_F3, '\r'): /* 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, '\r'): /* Teclast X98+ II firmware sends malformed events */ case KEYPRESS(0, SCAN_RIGHT, 0): + case KEYPRESS(0, SCAN_SUSPEND, 0): /* Handle phones/tablets with only a power key + volume up/down rocker (and otherwise just touchscreen input) */ action = ACTION_RUN; break; From 38faff48e59284c4bdab9e10d604585bac402171 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 3 Jul 2024 15:36:28 +0200 Subject: [PATCH 11/12] boot: don't set OsIndications field if already set correctly --- src/boot/efi/boot.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index b281136c05..6333cdade6 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -620,16 +620,21 @@ static void print_status(Config *config, char16_t *loaded_image_path) { } static EFI_STATUS set_reboot_into_firmware(void) { - uint64_t osind = 0; EFI_STATUS err; + uint64_t osind = 0; (void) efivar_get_uint64_le(MAKE_GUID_PTR(EFI_GLOBAL_VARIABLE), u"OsIndications", &osind); + + if (FLAGS_SET(osind, EFI_OS_INDICATIONS_BOOT_TO_FW_UI)) + return EFI_SUCCESS; + osind |= EFI_OS_INDICATIONS_BOOT_TO_FW_UI; err = efivar_set_uint64_le(MAKE_GUID_PTR(EFI_GLOBAL_VARIABLE), u"OsIndications", osind, EFI_VARIABLE_NON_VOLATILE); if (err != EFI_SUCCESS) - log_error_status(err, "Error setting OsIndications: %m"); - return err; + return log_error_status(err, "Error setting OsIndications, ignoring: %m"); + + return EFI_SUCCESS; } _noreturn_ static EFI_STATUS poweroff_system(void) { From 89ed34459eb67066f13e4133532b5adb2828c93b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 3 Jul 2024 16:21:34 +0200 Subject: [PATCH 12/12] ukify: bring order of EFI sections in man + --help into same order as spec Previously, the order was quite chaotic, even sometimes interleaved with entirely unrelated switches. Let's clean this up and use the same order as in the spec. This doesn't change anything real, but I think it's a worthy clean-up in particular as this order is documented as the PCR measurement order of these sections, hence there's actually a bit of relevance to always communicate the same order everywhere. --- man/ukify.xml | 104 ++++++++++++++++++++++----------------------- src/ukify/ukify.py | 91 ++++++++++++++++++++------------------- 2 files changed, 99 insertions(+), 96 deletions(-) diff --git a/man/ukify.xml b/man/ukify.xml index bf6f328536..68c72b0ba1 100644 --- a/man/ukify.xml +++ b/man/ukify.xml @@ -292,6 +292,29 @@ + + OSRelease=TEXT|@PATH + + + The os-release description (the .osrel section). The argument + may be a literal string, or @ followed by a path name. If not specified, the + os-release5 file + will be picked up from the host system. + + + + + + Cmdline=TEXT|@PATH + + + The kernel command line (the .cmdline section). The argument may + be a literal string, or @ followed by a path name. If not specified, no command + line will be embedded. + + + + Initrd=INITRD... @@ -314,24 +337,12 @@ - Cmdline=TEXT|@PATH - + Splash=PATH + - The kernel command line (the .cmdline section). The argument may - be a literal string, or @ followed by a path name. If not specified, no command - line will be embedded. - - - - - - OSRelease=TEXT|@PATH - - - The os-release description (the .osrel section). The argument - may be a literal string, or @ followed by a path name. If not specified, the - os-release5 file - will be picked up from the host system. + A picture to display during boot (the .splash section). The + argument is a path to a BMP file. If not specified, the section will not be present. + @@ -348,16 +359,35 @@ - Splash=PATH - + Uname=VERSION + - A picture to display during boot (the .splash section). The - argument is a path to a BMP file. If not specified, the section will not be present. - + Specify the kernel version (as in uname -r, the + .uname section). If not specified, an attempt will be made to extract the + version string from the kernel image. It is recommended to pass this explicitly if known, because + the extraction is based on heuristics and not very reliable. If not specified and extraction fails, + the section will not be present. + + SBAT=TEXT|@PATH + + + SBAT metadata associated with the UKI or addon. SBAT policies are useful to revoke + whole groups of UKIs or addons with a single, static policy update that does not take space in + DBX/MOKX. If not specified manually, a default metadata entry consisting of + uki,1,UKI,uki,1,https://uapi-group.org/specifications/specs/unified_kernel_image/ + for UKIs and + uki-addon,1,UKI Addon,addon,1,https://www.freedesktop.org/software/systemd/man/latest/systemd-stub.html + for addons will be used, to ensure it is always possible to revoke them. For more information on + SBAT see Shim documentation. + + + + + PCRPKey=PATH @@ -370,19 +400,6 @@ - - Uname=VERSION - - - Specify the kernel version (as in uname -r, the - .uname section). If not specified, an attempt will be made to extract the - version string from the kernel image. It is recommended to pass this explicitly if known, because - the extraction is based on heuristics and not very reliable. If not specified and extraction fails, - the section will not be present. - - - - PCRBanks=PATH @@ -488,23 +505,6 @@ - - - SBAT=TEXT|@PATH - - - SBAT metadata associated with the UKI or addon. SBAT policies are useful to revoke - whole groups of UKIs or addons with a single, static policy update that does not take space in - DBX/MOKX. If not specified manually, a default metadata entry consisting of - uki,1,UKI,uki,1,https://uapi-group.org/specifications/specs/unified_kernel_image/ - for UKIs and - uki-addon,1,UKI Addon,addon,1,https://www.freedesktop.org/software/systemd/man/latest/systemd-stub.html - for addons will be used, to ensure it is always possible to revoke them. For more information on - SBAT see Shim documentation. - - - - diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index 76437f25e5..5a36ce06ee 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -1264,6 +1264,13 @@ CONFIG_ITEMS = [ action = 'store_true', ), + ConfigItem( + ('--config', '-c'), + metavar = 'PATH', + type = pathlib.Path, + help = 'configuration file', + ), + ConfigItem( '--linux', type = pathlib.Path, @@ -1271,6 +1278,20 @@ CONFIG_ITEMS = [ config_key = 'UKI/Linux', ), + ConfigItem( + '--os-release', + metavar = 'TEXT|@PATH', + help = 'path to os-release file [.osrel section]', + config_key = 'UKI/OSRelease', + ), + + ConfigItem( + '--cmdline', + metavar = 'TEXT|@PATH', + help = 'kernel command line [.cmdline section]', + config_key = 'UKI/Cmdline', + ), + ConfigItem( '--initrd', metavar = 'INITRD', @@ -1290,24 +1311,11 @@ CONFIG_ITEMS = [ ), ConfigItem( - ('--config', '-c'), - metavar = 'PATH', + '--splash', + metavar = 'BMP', type = pathlib.Path, - help = 'configuration file', - ), - - ConfigItem( - '--cmdline', - metavar = 'TEXT|@PATH', - help = 'kernel command line [.cmdline section]', - config_key = 'UKI/Cmdline', - ), - - ConfigItem( - '--os-release', - metavar = 'TEXT|@PATH', - help = 'path to os-release file [.osrel section]', - config_key = 'UKI/OSRelease', + help = 'splash image bitmap file [.splash section]', + config_key = 'UKI/Splash', ), ConfigItem( @@ -1317,13 +1325,23 @@ CONFIG_ITEMS = [ help = 'Device Tree file [.dtb section]', config_key = 'UKI/DeviceTree', ), + ConfigItem( - '--splash', - metavar = 'BMP', - type = pathlib.Path, - help = 'splash image bitmap file [.splash section]', - config_key = 'UKI/Splash', + '--uname', + metavar='VERSION', + help='"uname -r" information [.uname section]', + config_key = 'UKI/Uname', ), + + ConfigItem( + '--sbat', + metavar = 'TEXT|@PATH', + help = 'SBAT policy [.sbat section]', + default = [], + action = 'append', + config_key = 'UKI/SBAT', + ), + ConfigItem( '--pcrpkey', metavar = 'KEY', @@ -1331,11 +1349,14 @@ CONFIG_ITEMS = [ help = 'embedded public key to seal secrets to [.pcrpkey section]', config_key = 'UKI/PCRPKey', ), + ConfigItem( - '--uname', - metavar='VERSION', - help='"uname -r" information [.uname section]', - config_key = 'UKI/Uname', + '--section', + dest = 'sections', + metavar = 'NAME:TEXT|@PATH', + action = 'append', + default = [], + help = 'section as name and contents [NAME section] or section to print', ), ConfigItem( @@ -1353,24 +1374,6 @@ CONFIG_ITEMS = [ config_key = 'UKI/Stub', ), - ConfigItem( - '--sbat', - metavar = 'TEXT|@PATH', - help = 'SBAT policy [.sbat section]', - default = [], - action = 'append', - config_key = 'UKI/SBAT', - ), - - ConfigItem( - '--section', - dest = 'sections', - metavar = 'NAME:TEXT|@PATH', - action = 'append', - default = [], - help = 'section as name and contents [NAME section] or section to print', - ), - ConfigItem( '--pcr-banks', metavar = 'BANK…',