From 88e26303ce922bb20327e62cd8fbfa3c997384cd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Nov 2025 12:12:30 +0100 Subject: [PATCH 1/5] efivars: don't bother with realloc() if we have no interest in the old data We shouldn't ask glibc to keep the old data around (which realloc() is about), given we overwrite it entirely anyway. Let's hence speed things up here, and allow glibc to just allocate a new block for us (and shorten the code a bit) --- src/basic/efivars.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/basic/efivars.c b/src/basic/efivars.c index b5e12eda62..9687f3d0e6 100644 --- a/src/basic/efivars.c +++ b/src/basic/efivars.c @@ -81,10 +81,10 @@ int efi_get_variable( } /* We want +1 for the read call, and +3 for the additional terminating bytes added below. */ - char *t = realloc(buf, (size_t) st.st_size + MAX(1, 3)); - if (!t) + free(buf); + buf = malloc((size_t) st.st_size + MAX(1, 3)); + if (!buf) return -ENOMEM; - buf = t; const struct iovec iov[] = { { &attr, sizeof(attr) }, From ab69a04600fd34c152c44be6864eb3bc64568e17 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Nov 2025 12:14:34 +0100 Subject: [PATCH 2/5] efivars: fix size checks in efi_get_variable() writev() returns the full size, not just the payload size, hence always add sizeof(attr) where necessary. Let's also change a couple of "4" into sizeof(attr) all over the place, to make clear what they are about. Fixes: #39695 Follow-up for: 9db9d6806e398465a6366dfc5bdde2e24338ac29 --- src/basic/efivars.c | 58 ++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/src/basic/efivars.c b/src/basic/efivars.c index 9687f3d0e6..c50983bdfc 100644 --- a/src/basic/efivars.c +++ b/src/basic/efivars.c @@ -66,13 +66,12 @@ int efi_get_variable( if (fstat(fd, &st) < 0) return log_debug_errno(errno, "fstat(\"%s\") failed: %m", p); - if (st.st_size == 0) - return log_debug_errno(SYNTHETIC_ERRNO(ENOENT), - "EFI variable %s is uncommitted", p); - if (st.st_size < 4) - return log_debug_errno(SYNTHETIC_ERRNO(ENODATA), "EFI variable %s is shorter than 4 bytes, refusing.", p); - if (st.st_size > 4*1024*1024 + 4) - return log_debug_errno(SYNTHETIC_ERRNO(E2BIG), "EFI variable %s is ridiculously large, refusing.", p); + if (st.st_size == 0) /* for uncommited variables, see below */ + return log_debug_errno(SYNTHETIC_ERRNO(ENOENT), "EFI variable '%s' is uncommitted", p); + if ((uint64_t) st.st_size < sizeof(attr)) + return log_debug_errno(SYNTHETIC_ERRNO(ENODATA), "EFI variable '%s' is shorter than %zu bytes, refusing.", p, sizeof(attr)); + if ((uint64_t) st.st_size > sizeof(attr) + 4 * U64_MB) + return log_debug_errno(SYNTHETIC_ERRNO(E2BIG), "EFI variable '%s' is ridiculously large, refusing.", p); if (!ret_attribute && !ret_value) { /* No need to read anything, return the reported size. */ @@ -82,28 +81,31 @@ int efi_get_variable( /* We want +1 for the read call, and +3 for the additional terminating bytes added below. */ free(buf); - buf = malloc((size_t) st.st_size + MAX(1, 3)); + buf = malloc((size_t) st.st_size - sizeof(attr) + CONST_MAX(1, 3)); if (!buf) return -ENOMEM; - const struct iovec iov[] = { - { &attr, sizeof(attr) }, - { buf, (size_t) st.st_size + 1 }, + struct iovec iov[] = { + { &attr, sizeof(attr) }, + { buf, (size_t) st.st_size - sizeof(attr) + 1 }, }; n = readv(fd, iov, 2); - assert(n <= st.st_size + 1); - if (n == st.st_size + 1) - /* We need to try again with a bigger buffer. */ - continue; - if (n >= 0) - break; + if (n < 0) { + if (errno != EINTR) + return log_debug_errno(errno, "Reading from '%s' failed: %m", p); + + log_debug("Reading from '%s' failed with EINTR, retrying.", p); + } else if ((size_t) n == sizeof(attr) + st.st_size + 1) + /* We need to try again with a bigger buffer, the variable was apparently changed concurrently? */ + log_debug("EFI variable '%s' larger than expected, retrying.", p); + else { + assert((size_t) n < sizeof(attr) + st.st_size + 1); + break; + } - log_debug_errno(errno, "Reading from \"%s\" failed: %m", p); - if (errno != EINTR) - return -errno; if (try >= EFI_N_RETRIES_TOTAL) - return -EBUSY; + return log_debug_errno(SYNTHETIC_ERRNO(EBUSY), "Reading EFI variable '%s' failed even after %u tries, giving up.", p, try); if (try >= EFI_N_RETRIES_NO_DELAY) (void) usleep_safe(EFI_RETRY_DELAY); } @@ -122,19 +124,21 @@ int efi_get_variable( if (n == 0) return log_debug_errno(SYNTHETIC_ERRNO(ENOENT), "EFI variable %s is uncommitted", p); - if (n < 4) + if ((size_t) n < sizeof(attr)) return log_debug_errno(SYNTHETIC_ERRNO(EIO), - "Read %zi bytes from EFI variable %s, expected >= 4", n, p); + "Read %zi bytes from EFI variable %s, expected >= %zu", n, p, sizeof(attr)); + size_t value_size = n - sizeof(attr); if (ret_attribute) *ret_attribute = attr; + if (ret_value) { assert(buf); /* Always NUL-terminate (3 bytes, to properly protect UTF-16, even if truncated in * the middle of a character) */ - buf[n - 4] = 0; - buf[n - 4 + 1] = 0; - buf[n - 4 + 2] = 0; + buf[value_size] = 0; + buf[value_size + 1] = 0; + buf[value_size + 2] = 0; *ret_value = TAKE_PTR(buf); } @@ -149,7 +153,7 @@ int efi_get_variable( * with a smaller value. */ if (ret_size) - *ret_size = n - 4; + *ret_size = value_size; return 0; } From 40cb2aa4f81b6b2af198f7c645abbf4f549c0f2e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Nov 2025 12:33:12 +0100 Subject: [PATCH 3/5] efivars: validate we are actually talking about a regular file We already have the stat data, let's actually check if things are alright before relying on .st_size --- src/basic/efivars.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/basic/efivars.c b/src/basic/efivars.c index c50983bdfc..f40c8a09e0 100644 --- a/src/basic/efivars.c +++ b/src/basic/efivars.c @@ -14,6 +14,7 @@ #include "io-util.h" #include "log.h" #include "memory-util.h" +#include "stat-util.h" #include "string-util.h" #include "time-util.h" #include "utf8.h" @@ -32,6 +33,7 @@ int efi_get_variable( void **ret_value, size_t *ret_size) { + int r; usec_t begin = 0; /* Unnecessary initialization to appease gcc */ assert(variable); @@ -66,6 +68,10 @@ int efi_get_variable( if (fstat(fd, &st) < 0) return log_debug_errno(errno, "fstat(\"%s\") failed: %m", p); + r = stat_verify_regular(&st); + if (r < 0) + return log_debug_errno(r, "EFI variable '%s' is not a regular file, refusing: %m", p); + if (st.st_size == 0) /* for uncommited variables, see below */ return log_debug_errno(SYNTHETIC_ERRNO(ENOENT), "EFI variable '%s' is uncommitted", p); if ((uint64_t) st.st_size < sizeof(attr)) From dbc25d84aeff8e9196c002a778fbaf91d979a1b9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Nov 2025 12:35:36 +0100 Subject: [PATCH 4/5] efivars: seek back to beginning in each efi_get_variable() loop We try to read again from the beginning, hence let's seek back. Apparently efivarfs doesn't strictly require this, but it's really weird that it doesn't. --- src/basic/efivars.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/basic/efivars.c b/src/basic/efivars.c index f40c8a09e0..f54c3d22e9 100644 --- a/src/basic/efivars.c +++ b/src/basic/efivars.c @@ -114,6 +114,9 @@ int efi_get_variable( return log_debug_errno(SYNTHETIC_ERRNO(EBUSY), "Reading EFI variable '%s' failed even after %u tries, giving up.", p, try); if (try >= EFI_N_RETRIES_NO_DELAY) (void) usleep_safe(EFI_RETRY_DELAY); + + /* Start from the beginning */ + (void) lseek(fd, 0, SEEK_SET); } /* Unfortunately kernel reports EOF if there's an inconsistency between efivarfs var list and From f5452477d40cfe2f971dbcb5a98f2888d5fac640 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Nov 2025 14:15:33 +0100 Subject: [PATCH 5/5] tree-wide: fix lseek() parameter order The offset must be specified first, 'whence' second. Fix that. Except for one case this fix doesn't actually fix any real bug, since SEEK_SET is defined as 0 anyway, hence the swapped arguments have no effect. The one exception is the MTD smartmedia code, which I guess indicates that noone has been using that hw anymore in a long time? --- src/home/homed-home.c | 2 +- src/import/pull-raw.c | 2 +- src/login/logind-seat.c | 4 ++-- src/test/test-copy.c | 2 +- src/test/test-process-util.c | 4 ++-- src/udev/mtd_probe/probe_smartmedia.c | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/home/homed-home.c b/src/home/homed-home.c index a359baea59..e4f3487473 100644 --- a/src/home/homed-home.c +++ b/src/home/homed-home.c @@ -570,7 +570,7 @@ static int home_parse_worker_stdout(int _fd, UserRecord **ret) { return 0; } - if (lseek(fd, SEEK_SET, 0) < 0) + if (lseek(fd, 0, SEEK_SET) < 0) return log_error_errno(errno, "Failed to seek to beginning of memfd: %m"); f = take_fdopen(&fd, "r"); diff --git a/src/import/pull-raw.c b/src/import/pull-raw.c index 80ef38f50a..01a810a9b8 100644 --- a/src/import/pull-raw.c +++ b/src/import/pull-raw.c @@ -365,7 +365,7 @@ static int raw_pull_make_local_copy(RawPull *i) { assert(i->raw_job->disk_fd >= 0); assert(i->offset == UINT64_MAX); - if (lseek(i->raw_job->disk_fd, SEEK_SET, 0) < 0) + if (lseek(i->raw_job->disk_fd, 0, SEEK_SET) < 0) return log_error_errno(errno, "Failed to seek to beginning of vendor image: %m"); } diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index d30cc6974f..3a3148be14 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -570,8 +570,8 @@ int seat_read_active_vt(Seat *s) { if (!seat_has_vts(s)) return 0; - if (lseek(s->manager->console_active_fd, SEEK_SET, 0) < 0) - return log_error_errno(errno, "lseek on console_active_fd failed: %m"); + if (lseek(s->manager->console_active_fd, 0, SEEK_SET) < 0) + return log_error_errno(errno, "lseek() on console_active_fd failed: %m"); errno = 0; k = read(s->manager->console_active_fd, t, sizeof(t)-1); diff --git a/src/test/test-copy.c b/src/test/test-copy.c index 86de5c4b63..58797b5926 100644 --- a/src/test/test-copy.c +++ b/src/test/test-copy.c @@ -124,7 +124,7 @@ TEST(copy_file_fd) { assert_se(write_string_file(in_fn, text, WRITE_STRING_FILE_CREATE) == 0); assert_se(copy_file_fd("/a/file/which/does/not/exist/i/guess", out_fd, COPY_REFLINK) < 0); assert_se(copy_file_fd(in_fn, out_fd, COPY_REFLINK) >= 0); - assert_se(lseek(out_fd, SEEK_SET, 0) == 0); + assert_se(lseek(out_fd, 0, SEEK_SET) == 0); assert_se(read(out_fd, buf, sizeof buf) == (ssize_t) strlen(text)); ASSERT_STREQ(buf, text); diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index 5723f2a074..b797845334 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -525,7 +525,7 @@ TEST(pid_get_cmdline_harder) { #define EXPECT1p "foo $'\\'bar\\'' $'\"bar$\"' $'x y z' $'!``'" #define EXPECT1v STRV_MAKE("foo", "'bar'", "\"bar$\"", "x y z", "!``") - ASSERT_OK_ZERO_ERRNO(lseek(fd, SEEK_SET, 0)); + ASSERT_OK_ZERO_ERRNO(lseek(fd, 0, SEEK_SET)); ASSERT_OK_EQ_ERRNO(write(fd, CMDLINE1, sizeof(CMDLINE1)), (ssize_t) sizeof(CMDLINE1)); ASSERT_OK_ZERO_ERRNO(ftruncate(fd, sizeof(CMDLINE1))); @@ -550,7 +550,7 @@ TEST(pid_get_cmdline_harder) { #define EXPECT2p "foo $'\\001\\002\\003'" #define EXPECT2v STRV_MAKE("foo", "\1\2\3") - ASSERT_OK_ZERO_ERRNO(lseek(fd, SEEK_SET, 0)); + ASSERT_OK_ZERO_ERRNO(lseek(fd, 0, SEEK_SET)); ASSERT_OK_EQ_ERRNO(write(fd, CMDLINE2, sizeof(CMDLINE2)), (ssize_t) sizeof(CMDLINE2)); ASSERT_OK_ZERO_ERRNO(ftruncate(fd, sizeof CMDLINE2)); diff --git a/src/udev/mtd_probe/probe_smartmedia.c b/src/udev/mtd_probe/probe_smartmedia.c index 9ce49b8552..e2c325ac74 100644 --- a/src/udev/mtd_probe/probe_smartmedia.c +++ b/src/udev/mtd_probe/probe_smartmedia.c @@ -69,7 +69,7 @@ int probe_smart_media(int mtd_fd, mtd_info_t* info) { } for (offset = 0; offset < block_size * spare_count; offset += sector_size) { - (void) lseek(mtd_fd, SEEK_SET, offset); + (void) lseek(mtd_fd, offset, SEEK_SET); if (read(mtd_fd, cis_buffer, SM_SECTOR_SIZE) == SM_SECTOR_SIZE) { cis_found = 1;