From: Lennart Poettering Date: Fri, 22 May 2026 02:34:54 +0000 (+0200) Subject: bootctl: refactor get_file_version() to use proper PE parsing X-Git-Tag: v261-rc1~7^2 X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=e3e897cf8fe56d5ff48a8cf56fdcfdfc010db0f5;p=thirdparty%2Fsystemd.git bootctl: refactor get_file_version() to use proper PE parsing This irked me for a while. Let's not scan for strings stupidly, but properly parse PE files to find the magic marker. It's easy with our own PE APIs, hence we should do it. This moves logging to the callers (previously, this was all mixed up). --- diff --git a/src/bootctl/bootctl-install.c b/src/bootctl/bootctl-install.c index b609d8f227c..5fd65e02f15 100644 --- a/src/bootctl/bootctl-install.c +++ b/src/bootctl/bootctl-install.c @@ -462,13 +462,13 @@ static int version_check(int fd_from, const char *from, int fd_to, const char *t if (r == -ESRCH) return log_notice_errno(r, "Source file \"%s\" does not carry version information!", from); if (r < 0) - return r; + return log_error_errno(r, "Failed to get file version of '%s': %m", from); r = get_file_version(fd_to, &b); if (r == -ESRCH) return log_info_errno(r, "Skipping \"%s\", it's owned by another boot loader (no version info found).", to); if (r < 0) - return r; + return log_error_errno(r, "Failed to get file version of '%s': %m", to); if (compare_product(a, b) != 0) return log_info_errno(SYNTHETIC_ERRNO(ESRCH), "Skipping \"%s\", it's owned by another boot loader.", to); @@ -1806,6 +1806,8 @@ static int remove_boot_efi(InstallContext *c) { return log_oom(); fd = xopenat_full(dirfd(d), de->d_name, O_RDONLY|O_CLOEXEC|O_NONBLOCK|O_NOCTTY|O_NOFOLLOW, XO_REGULAR, /* mode= */ MODE_INVALID); + if (fd == -ENOENT) + continue; if (fd < 0) return log_error_errno(fd, "Failed to open '%s' for reading: %m", z); @@ -1821,8 +1823,10 @@ static int remove_boot_efi(InstallContext *c) { r = get_file_version(fd, &v); if (r == -ESRCH) continue; /* No version information */ - if (r < 0) - return r; + if (r < 0) { + log_warning_errno(r, "Failed to get file version of '%s', skipping: %m", de->d_name); + continue; + } if (!startswith(v, "systemd-boot ")) continue; diff --git a/src/bootctl/bootctl-status.c b/src/bootctl/bootctl-status.c index f1c75c4a65a..8ac24447442 100644 --- a/src/bootctl/bootctl-status.c +++ b/src/bootctl/bootctl-status.c @@ -217,13 +217,15 @@ static int enumerate_binaries( return log_oom(); LOG_SET_PREFIX(filename); - fd = openat(dirfd(d), de->d_name, O_RDONLY|O_CLOEXEC); + fd = RET_NERRNO(openat(dirfd(d), de->d_name, O_RDONLY|O_CLOEXEC)); + if (fd == -ENOENT) + continue; if (fd < 0) - return log_error_errno(errno, "Failed to open file for reading: %m"); + return log_error_errno(fd, "Failed to open file '%s' for reading: %m", de->d_name); r = get_file_version(fd, &v); if (r < 0 && r != -ESRCH) - return r; + return log_error_errno(r, "Failed to get file version of '%s': %m", de->d_name); if (*previous) { /* Let's output the previous entry now, since now we know that there will be * one more, and can draw the tree glyph properly. */ @@ -237,7 +239,7 @@ static int enumerate_binaries( /* Do not output this entry immediately, but store what should be printed in a state * variable, because we only will know the tree glyph to print (branch or final edge) once we * read one more entry */ - if (r == -ESRCH) /* No systemd-owned file but still interesting to print */ + if (r == -ESRCH) /* Not systemd-owned file but still interesting to print */ r = asprintf(previous, "%s%s/%s/%s/%s", ansi_grey(), esp_path, ansi_normal(), path, de->d_name); else /* if (r >= 0) */ diff --git a/src/bootctl/bootctl-util.c b/src/bootctl/bootctl-util.c index 9ed31372bf4..9bd85d64614 100644 --- a/src/bootctl/bootctl-util.c +++ b/src/bootctl/bootctl-util.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include -#include #include #include "alloc-util.h" @@ -12,7 +11,7 @@ #include "errno-util.h" #include "fileio.h" #include "log.h" -#include "stat-util.h" +#include "pe-binary.h" #include "string-util.h" #include "sync-util.h" #include "virt.h" @@ -133,61 +132,60 @@ const char* get_efi_arch(void) { return EFI_MACHINE_TYPE_NAME; } -/* search for "#### LoaderInfo: systemd-boot 218 ####" string inside the binary */ int get_file_version(int fd, char **ret) { - struct stat st; - char *buf; - const char *s, *e; - char *marker = NULL; int r; assert(fd >= 0); assert(ret); - /* Does not reposition file offset (as it uses mmap()) */ + /* Reads the version marker that systemd-boot/systemd-stub and friends store in their ".sdmagic" PE + * section, i.e. a string such as "#### LoaderInfo: systemd-boot 218 ####", and returns the inner + * part, e.g. "systemd-boot 218". Does not reposition the file offset (as it uses pread()). */ - if (fstat(fd, &st) < 0) - return log_error_errno(errno, "Failed to stat EFI binary: %m"); - - r = stat_verify_regular(&st); - if (r < 0) { - log_debug_errno(r, "EFI binary is not a regular file, assuming no version information: %m"); - return -ESRCH; - } + _cleanup_free_ IMAGE_DOS_HEADER *dos_header = NULL; + _cleanup_free_ PeHeader *pe_header = NULL; + r = pe_load_headers(fd, &dos_header, &pe_header); + if (r == -EBADMSG) + return log_debug_errno(SYNTHETIC_ERRNO(ESRCH), "EFI binary is not a valid PE file, assuming no version information."); + if (r < 0) + return r; - if (st.st_size < 27 || file_offset_beyond_memory_size(st.st_size)) - return log_debug_errno(SYNTHETIC_ERRNO(ESRCH), - "EFI binary size too %s: %"PRIi64, - st.st_size < 27 ? "small" : "large", st.st_size); + _cleanup_free_ IMAGE_SECTION_HEADER *sections = NULL; + r = pe_load_sections(fd, dos_header, pe_header, §ions); + if (r == -EBADMSG) + return log_debug_errno(SYNTHETIC_ERRNO(ESRCH), "Failed to load PE section table, assuming no version information."); + if (r < 0) + return r; - buf = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0); - if (buf == MAP_FAILED) - return log_error_errno(errno, "Failed to mmap EFI binary: %m"); + _cleanup_free_ char *sdmagic = NULL; + r = pe_read_section_data_by_name( + fd, + pe_header, + sections, + ".sdmagic", + /* max_size= */ 4U*1024U, + (void**) &sdmagic, + /* ret_size= */ NULL); + if (IN_SET(r, -ENXIO, -EBADMSG)) + return log_debug_errno(SYNTHETIC_ERRNO(ESRCH), "EFI binary has no .sdmagic section, assuming no version information."); + if (r < 0) + return log_debug_errno(r, "Failed to read .sdmagic section of EFI binary: %m"); - s = mempmem_safe(buf, st.st_size - 8, "#### LoaderInfo: ", 17); - if (!s) { - r = log_debug_errno(SYNTHETIC_ERRNO(ESRCH), "EFI binary has no LoaderInfo marker."); - goto finish; - } + const char *p = startswith(sdmagic, "#### LoaderInfo: "); + if (!p) + return log_debug_errno(SYNTHETIC_ERRNO(ESRCH), "EFI binary .sdmagic section lacks LoaderInfo marker."); - e = memmem_safe(s, st.st_size - (s - buf), " ####", 5); - if (!e || e - s < 3) { - r = log_error_errno(SYNTHETIC_ERRNO(EINVAL), "EFI binary has malformed LoaderInfo marker."); - goto finish; - } + const char *e = endswith(p, " ####"); + if (!e || e <= p) + return log_debug_errno(SYNTHETIC_ERRNO(ESRCH), "EFI binary has malformed LoaderInfo marker."); - marker = strndup(s, e - s); - if (!marker) { - r = log_oom(); - goto finish; - } + char *marker = strndup(p, e - p); + if (!marker) + return log_oom_debug(); log_debug("EFI binary LoaderInfo marker: \"%s\"", marker); - r = 0; - *ret = marker; -finish: - (void) munmap(buf, st.st_size); - return r; + *ret = TAKE_PTR(marker); + return 0; } int settle_entry_token(void) { diff --git a/test/units/TEST-87-AUX-UTILS-VM.bootctl.sh b/test/units/TEST-87-AUX-UTILS-VM.bootctl.sh index 90daaf52ead..2b095c1fbcb 100755 --- a/test/units/TEST-87-AUX-UTILS-VM.bootctl.sh +++ b/test/units/TEST-87-AUX-UTILS-VM.bootctl.sh @@ -113,6 +113,48 @@ testcase_bootctl_basic() { basic_tests } +cleanup_file_version() { + if [[ -n "${FILE_VERSION_GARBAGE:-}" ]]; then + rm -f "$FILE_VERSION_GARBAGE" + unset FILE_VERSION_GARBAGE + fi + restore_esp +} + +testcase_bootctl_file_version() { + # Exercise get_file_version() (src/bootctl/bootctl-util.c), which reads the + # version marker that systemd-boot/-stub store in their ".sdmagic" PE + # section, e.g. "#### LoaderInfo: systemd-boot 257 ####". 'bootctl status' + # surfaces the inner part next to each .efi file it finds in the ESP, e.g. + # ".../systemd-bootx64.efi (systemd-boot 257)". + + backup_esp + trap cleanup_file_version RETURN ERR + + bootctl install + + local ESP + ESP="$(bootctl --print-esp-path)" + + # The freshly installed systemd-boot binary must be listed with the version + # extracted from its ".sdmagic" section. + bootctl status | grep -E "systemd-boot[a-z0-9]*\.efi \(systemd-boot [^)]+\)" >/dev/null + + # A non-PE/non-systemd file must be handled gracefully (get_file_version() + # returns -ESRCH): it is still listed, but without a version annotation, and + # 'bootctl status' must not fail. + FILE_VERSION_GARBAGE="$ESP/EFI/systemd/not-a-loader.efi" + echo "this is not a PE binary" >"$FILE_VERSION_GARBAGE" + + local status + status="$(bootctl status)" + grep -F "not-a-loader.efi" <<<"$status" >/dev/null + (! grep -E "not-a-loader\.efi .*\(" <<<"$status" >/dev/null) + + rm -f "$FILE_VERSION_GARBAGE" + unset FILE_VERSION_GARBAGE +} + cleanup_image() ( set +e