[PATCH v2 0/2] EFI: cfg file parsing adjustments

[PATCH v2 0/2] EFI: cfg file parsing adjustments

[Jan Beulich]

1: avoid OOB config file reads
2: move pre_parse() back next to get_value()

Jan

[PATCH v2 1/2] EFI: avoid OOB config file reads

[Jan Beulich]

The message emitted by pre_parse() pretty clearly states the original
intention. Yet what it said wasn't done, and would have been unfriendly to
the user. Hence accesses past the allocated buffer were possible. Insert a
terminating NUL immediately past the data read, to then drop the no longer
applicable message.

NB: The iscntrl() check of just the last byte is more strict than what
pre_parse() would accept without issuing its prior message, yet I'd like
to keep the new logic reasonably simple. Config files shouldn't be huge,
and we shouldn't be _that_ short of memory (or we'd fail elsewhere pretty
soon).

Fixes: bf6501a62e80 ("x86-64: EFI boot code")
Reported-by: Kamil Frankowicz <kamil.frankowicz@cert.pl>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Is the efi_arch_flush_dcache_area() really needed for config files? Else
it could be in an "else" to the "if()" added to read_file(). And then,
how is it guaranteed that data from the area isn't brought back into the
cache (perhaps speculatively)?

In read_section() we could further leverage section alignment padding (if
present, and if filled with zeroes), to limit when to allocate and copy.
Thoughts?
---
v2: Entirely different approach.

--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -833,8 +833,9 @@ static bool __init read_file(EFI_FILE_HA
     what = L"Allocation";
     file->addr = min(1UL << (32 + PAGE_SHIFT),
                      HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
+    /* For config files allocate an extra byte to put a NUL there. */
     ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
-                                PFN_UP(size), &file->addr);
+                                PFN_UP(size + (file == &cfg)), &file->addr);
     if ( EFI_ERROR(ret) )
         goto fail;
 
@@ -853,6 +854,9 @@ static bool __init read_file(EFI_FILE_HA
 
     efi_arch_flush_dcache_area(file->ptr, file->size);
 
+    if ( file == &cfg )
+        file->str[file->size] = 0;
+
     return true;
 
  fail:
@@ -878,6 +882,23 @@ static bool __init read_section(const EF
 
     file->ptr = ptr;
 
+    /* For cfg file, if necessary allocate space to put an extra NUL there. */
+    if ( file == &cfg && file->size && !iscntrl(file->str[file->size - 1]) )
+    {
+        EFI_PHYSICAL_ADDRESS addr;
+        EFI_STATUS ret = efi_bs->AllocatePages(AllocateMaxAddress,
+                                               EfiLoaderData,
+                                               PFN_UP(file->size + 1), &addr);
+
+        if ( EFI_ERROR(ret) )
+            return false;
+
+        memcpy((void *)addr, ptr, file->size);
+        file->addr = addr;
+        file->need_to_free = true;
+        file->str[file->size] = 0;
+    }
+
     handle_file_info(name, file, options);
 
     return true;
@@ -906,9 +927,6 @@ static void __init pre_parse(const struc
         else
             start = 0;
     }
-    if ( file->size && end[-1] )
-         PrintStr(L"No newline at end of config file,"
-                   " last line will be ignored.\r\n");
 }
 
 static void __init init_secure_boot_mode(void)

[PATCH v2 2/2] EFI: move pre_parse() back next to get_value()

[Jan Beulich]

These are siblings, so better would live close together. Sadly in
1f9540cd8eb6 ("EFI: move efi-boot.h inclusion point") I moved get_value()
without also moving pre_parse().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -581,6 +581,31 @@ static char * __init split_string(char *
     return NULL;
 }
 
+static void __init pre_parse(const struct file *file)
+{
+    char *ptr = file->str, *end = ptr + file->size;
+    bool start = true, comment = false;
+
+    for ( ; ptr < end; ++ptr )
+    {
+        if ( iscntrl(*ptr) )
+        {
+            comment = false;
+            start = true;
+            *ptr = 0;
+        }
+        else if ( comment || (start && isspace(*ptr)) )
+            *ptr = 0;
+        else if ( *ptr == '#' || (start && *ptr == ';') )
+        {
+            comment = true;
+            *ptr = 0;
+        }
+        else
+            start = 0;
+    }
+}
+
 static char *__init get_value(const struct file *file, const char *section,
                               const char *item)
 {
@@ -904,31 +929,6 @@ static bool __init read_section(const EF
     return true;
 }
 
-static void __init pre_parse(const struct file *file)
-{
-    char *ptr = file->str, *end = ptr + file->size;
-    bool start = true, comment = false;
-
-    for ( ; ptr < end; ++ptr )
-    {
-        if ( iscntrl(*ptr) )
-        {
-            comment = false;
-            start = true;
-            *ptr = 0;
-        }
-        else if ( comment || (start && isspace(*ptr)) )
-            *ptr = 0;
-        else if ( *ptr == '#' || (start && *ptr == ';') )
-        {
-            comment = true;
-            *ptr = 0;
-        }
-        else
-            start = 0;
-    }
-}
-
 static void __init init_secure_boot_mode(void)
 {
     static EFI_GUID __initdata gv_uuid = EFI_GLOBAL_VARIABLE;

Ping: [PATCH v2 0/2] EFI: cfg file parsing adjustments

[Jan Beulich]

On 25.03.2026 14:22, Jan Beulich wrote:
> 1: avoid OOB config file reads
> 2: move pre_parse() back next to get_value()

May I ask for feedback (preferably an ack, as you may guess)?

Jan

Re: [PATCH v2 0/2] EFI: cfg file parsing adjustments

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On Wed, Mar 25, 2026 at 02:22:57PM +0100, Jan Beulich wrote:
> 1: avoid OOB config file reads
> 2: move pre_parse() back next to get_value()

For the series:
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]