tools/libxg: Don't gunzip the guests initrd

Commit Message

Andrew Cooper Sept. 4, 2024, 12:31 p.m. UTC

When booting guests, compressed first executables (the kernel itself for
direct-boot, or firmware binaries) need decompressing in order to inspect the
ELF notes or other relevant headers.

However for unclear reasons, libxenguest will also gunzip() all modules which
happen to be compressed (typically a direct-boot kernel's initrd), before
loading the contents into guest memory.

This was an unexpected discovery while trying to test Xen's gunzip()
logic (Xen as a PVH guest, with a gzipped XTF kernel as dom0).

More generally, it is work that we should not be doing in the domain builder;
it can and should be done in guest context.

This is unlikely to cause any issues; a kernel incapable of decompression
wouldn't have a compressed initrd to start with, and these days there are
other algorithms typically preferred over gzip.

Furthermore, this change also fixes a bug with Linux guests.  Linux is happy
consuming a initrd constructed of multiple CPIO fragments, including with
differing compression, and Xen gunzip()'ing from the start would result in all
subsequent fragments being silently dropped during load.

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Anthony PERARD <[e-mail]>
CC: Juergen Gross <[e-mail]>
CC: Jan Beulich <[e-mail]>
CC: Stefano Stabellini <[e-mail]>
CC: Julien Grall <[e-mail]>
CC: Oleksii Kurochko <[e-mail]>

Furthermore, Linux's habit of prepending an uncompressed CPIO containing
microcode will also stop it being recognised as a compressed blob.
---
 CHANGELOG.md                   |  2 ++
 tools/libs/guest/xg_dom_core.c | 40 ++++++----------------------------
 2 files changed, 9 insertions(+), 33 deletions(-)

Comments

Jan Beulich Sept. 4, 2024, 12:39 p.m. UTC | #1

On 04.09.2024 14:31, Andrew Cooper wrote:
> When booting guests, compressed first executables (the kernel itself for
> direct-boot, or firmware binaries) need decompressing in order to inspect the
> ELF notes or other relevant headers.
> 
> However for unclear reasons, libxenguest will also gunzip() all modules which
> happen to be compressed (typically a direct-boot kernel's initrd), before
> loading the contents into guest memory.

I suppose the simple reason was that grub also decompresse{s,d} everything.

> This was an unexpected discovery while trying to test Xen's gunzip()
> logic (Xen as a PVH guest, with a gzipped XTF kernel as dom0).
> 
> More generally, it is work that we should not be doing in the domain builder;
> it can and should be done in guest context.
> 
> This is unlikely to cause any issues; a kernel incapable of decompression
> wouldn't have a compressed initrd to start with,

Unless relying on the bootloader, which an OS may certainly do.

> and these days there are
> other algorithms typically preferred over gzip.
> 
> Furthermore, this change also fixes a bug with Linux guests.  Linux is happy
> consuming a initrd constructed of multiple CPIO fragments, including with
> differing compression, and Xen gunzip()'ing from the start would result in all
> subsequent fragments being silently dropped during load.

Now this and perhaps also ...

> Signed-off-by: Andrew Cooper <[e-mail]>
> ---
> CC: Anthony PERARD <[e-mail]>
> CC: Juergen Gross <[e-mail]>
> CC: Jan Beulich <[e-mail]>
> CC: Stefano Stabellini <[e-mail]>
> CC: Julien Grall <[e-mail]>
> CC: Oleksii Kurochko <[e-mail]>
> 
> Furthermore, Linux's habit of prepending an uncompressed CPIO containing
> microcode will also stop it being recognised as a compressed blob.

... this are fair reasons on their own to get rid of that logic. So
let's hope there's not going to be any fallout.

Jan

Patch

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5521ae5bb37a..342d8c6073be 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,8 @@  The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 ## [4.20.0 UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
 
 ### Changed
+ - The dombuilder in libxenguest no longer un-gzips secondary modules, instead
+   leaving this to the guest kernel to do in guest context.
  - On x86:
    - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
 
diff --git a/tools/libs/guest/xg_dom_core.c b/tools/libs/guest/xg_dom_core.c
index f5521d528be1..595b0a667c03 100644
--- a/tools/libs/guest/xg_dom_core.c
+++ b/tools/libs/guest/xg_dom_core.c
@@ -980,37 +980,24 @@  int xc_dom_mem_init(struct xc_dom_image *dom, unsigned int mem_mb)
 
 static int xc_dom_build_module(struct xc_dom_image *dom, unsigned int mod)
 {
-    size_t unziplen, modulelen;
+    size_t modulelen;
     void *modulemap;
     char name[10];
 
-    if ( !dom->modules[mod].seg.vstart )
-        unziplen = xc_dom_check_gzip(dom->xch,
-                                     dom->modules[mod].blob, dom->modules[mod].size);
-    else
-        unziplen = 0;
+    modulelen = dom->modules[mod].size;
 
-    modulelen = max(unziplen, dom->modules[mod].size);
-    if ( dom->max_module_size )
+    if ( dom->max_module_size && modulelen > dom->max_module_size )
     {
-        if ( unziplen && modulelen > dom->max_module_size )
-        {
-            modulelen = min(unziplen, dom->modules[mod].size);
-            if ( unziplen > modulelen )
-                unziplen = 0;
-        }
-        if ( modulelen > dom->max_module_size )
-        {
-            xc_dom_panic(dom->xch, XC_INVALID_KERNEL,
-                         "module %u image too large", mod);
-            goto err;
-        }
+        xc_dom_panic(dom->xch, XC_INVALID_KERNEL,
+                     "module %u image too large", mod);
+        goto err;
     }
 
     snprintf(name, sizeof(name), "module%u", mod);
     if ( xc_dom_alloc_segment(dom, &dom->modules[mod].seg, name,
                               dom->modules[mod].seg.vstart, modulelen) != 0 )
         goto err;
+
     modulemap = xc_dom_seg_to_ptr(dom, &dom->modules[mod].seg);
     if ( modulemap == NULL )
     {
@@ -1018,21 +1005,8 @@  static int xc_dom_build_module(struct xc_dom_image *dom, unsigned int mod)
                   __FUNCTION__, mod);
         goto err;
     }
-    if ( unziplen )
-    {
-        if ( xc_dom_do_gunzip(dom->xch, dom->modules[mod].blob, dom->modules[mod].size,
-                              modulemap, unziplen) != -1 )
-            return 0;
-        if ( dom->modules[mod].size > modulelen )
-            goto err;
-    }
 
-    /* Fall back to handing over the raw blob. */
     memcpy(modulemap, dom->modules[mod].blob, dom->modules[mod].size);
-    /* If an unzip attempt was made, the buffer may no longer be all zero. */
-    if ( unziplen > dom->modules[mod].size )
-        memset(modulemap + dom->modules[mod].size, 0,
-               unziplen - dom->modules[mod].size);
 
     return 0;