boot: restore RW/RO memory attributes on every error

linux_exec() marks code sections RO+X for W^X and reverts them to RW+NX
in a loop just before returning, because EDK2 requires freed buffers to
be writable and non-executable or FreePages() crashes. Not every error
path is currently covered. Switch to a _cleanup_ helper so that every
return path is covered.

Follow-up for 56d19b633d049035afe3f690fd6c717e06f88597

diff --git a/src/boot/linux.c b/src/boot/linux.c
index 584fc051b71bf8b2afcf8b2bd2b80c2644f71913..56b932ecb563ddfd5bf10c4e609267178feea7ec 100644 (file)
--- a/src/boot/linux.c
+++ b/src/boot/linux.c
@@ -153,6 +153,25 @@ static EFI_STATUS memory_mark_rw_nx(EFI_MEMORY_ATTRIBUTE_PROTOCOL *memory_proto,
         return EFI_SUCCESS;
 }
 
+typedef struct CleanupNxSections {
+        EFI_MEMORY_ATTRIBUTE_PROTOCOL *memory_proto;
+        struct iovec *sections;
+        size_t n_sections;
+} CleanupNxSections;
+
+static void cleanup_nx_sections(CleanupNxSections *c) {
+        assert(c);
+
+        /* Restore the code sections that were marked RO+X back to RW+NX before their backing pages are
+         * freed: EDK2 requires freed buffers to be writable and non-executable (it may overwrite them with
+         * a fixed pattern), otherwise FreePages() crashes. */
+        if (c->memory_proto)
+                for (size_t i = 0; i < c->n_sections; i++)
+                        (void) memory_mark_rw_nx(c->memory_proto, &c->sections[i]);
+
+        free(c->sections);
+}
+
 EFI_STATUS linux_exec(
                 EFI_HANDLE parent_image,
                 const char16_t *cmdline,
@@ -242,8 +261,6 @@ EFI_STATUS linux_exec(
          * https://microsoft.github.io/mu/WhatAndWhy/enhancedmemoryprotection/
          * https://www.kraxel.org/blog/2023/12/uefi-nx-linux-boot/ */
         EFI_MEMORY_ATTRIBUTE_PROTOCOL *memory_proto = NULL;
-        _cleanup_free_ struct iovec *nx_sections = NULL;
-        size_t n_nx_sections = 0;
 
         if (pe_kernel_check_nx_compat(kernel->iov_base)) {
                 /* LocateProtocol() is not quite that quick if you have many protocols, so only look for it
@@ -276,6 +293,12 @@ EFI_STATUS linux_exec(
                         /* addr= */ 0);
 
         uint8_t* loaded_kernel = PHYSICAL_ADDRESS_TO_POINTER(loaded_kernel_pages.addr);
+
+        /* Any code section marked RO+X must be reverted to RW+NX before the backing pages are freed. */
+        _cleanup_(cleanup_nx_sections) CleanupNxSections nx_restore = {
+                .memory_proto = memory_proto,
+        };
+
         FOREACH_ARRAY(h, headers, n_headers) {
                 if (h->PointerToRelocations != 0)
                         return log_error_status(EFI_LOAD_ERROR, "Inner kernel image contains sections with relocations, which we do not support.");
@@ -304,15 +327,14 @@ EFI_STATUS linux_exec(
 
                 /* Not a code section? Nothing to do, leave as-is. */
                 if (memory_proto && (h->Characteristics & (PE_CODE|PE_EXECUTE))) {
-                        nx_sections = xrealloc(nx_sections, n_nx_sections * sizeof(struct iovec), (n_nx_sections + 1) * sizeof(struct iovec));
-                        nx_sections[n_nx_sections].iov_base = loaded_kernel + h->VirtualAddress;
-                        nx_sections[n_nx_sections].iov_len = h->VirtualSize;
+                        /* Record the section for cleanup before marking it RO+X: if memory_mark_ro_x()
+                         * fails after partially applying the attributes, cleanup still reverts them. */
+                        nx_restore.sections = xrealloc(nx_restore.sections, nx_restore.n_sections * sizeof(struct iovec), (nx_restore.n_sections + 1) * sizeof(struct iovec));
+                        nx_restore.sections[nx_restore.n_sections++] = IOVEC_MAKE(loaded_kernel + h->VirtualAddress, h->VirtualSize);
 
-                        err = memory_mark_ro_x(memory_proto, &nx_sections[n_nx_sections]);
+                        err = memory_mark_ro_x(memory_proto, &nx_restore.sections[nx_restore.n_sections - 1]);
                         if (err != EFI_SUCCESS)
                                 return err;
-
-                        ++n_nx_sections;
                 }
         }
 
@@ -352,11 +374,5 @@ EFI_STATUS linux_exec(
         /* Restore */
         *parent_loaded_image = original_parent_loaded_image;
 
-        /* On failure we'll free the buffers. EDK2 requires the memory buffers to be writable and
-         * non-executable, as in some configurations it will overwrite them with a fixed pattern, so if the
-         * attributes are not restored FreePages() will crash. */
-        for (size_t i = 0; i < n_nx_sections; i++)
-                (void) memory_mark_rw_nx(memory_proto, &nx_sections[i]);
-
         return log_error_status(err, "Error starting kernel image: %m");
 }