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

diff --git a/src/basic/efivars.c b/src/basic/efivars.c
index 9687f3d0e642a5e2701926b12aabf035e09c3fb5..c50983bdfcc581e8e4e56eefd1735c4e05111143 100644 (file)
--- 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)
+                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;
 }