[RFC PATCH] hw: arm: Support direct boot for Linux/arm64 EFI zboot images

[RFC PATCH] hw: arm: Support direct boot for Linux/arm64 EFI zboot images

[Ard Biesheuvel]

Fedora 39 will ship its arm64 kernels in the new generic EFI zboot
format, using gzip compression for the payload.

For doing EFI boot in QEMU, this is completely transparent, as the
firmware or bootloader will take care of this. However, for direct
kernel boot without firmware, we will lose the ability to boot such
distro kernels unless we deal with the new format directly.

EFI zboot images contain metadata in the header regarding the placement
of the compressed payload inside the image, and the type of compression
used. This means we can wire up the existing gzip support without too
much hassle, by parsing the header and grabbing the payload from inside
the loaded zboot image.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 hw/arm/boot.c       |  4 ++
 hw/core/loader.c    | 64 ++++++++++++++++++++
 include/hw/loader.h |  2 +
 3 files changed, 70 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 3d7d11f782feb5da..dc10a0788227443e 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -924,6 +924,10 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
         size = len;
     }
 
+    if (unpack_efi_zboot_image(&buffer, &size)) {
+        return -1;
+    }
+
     /* check the arm64 magic header value -- very old kernels may not have it */
     if (size > ARM64_MAGIC_OFFSET + 4 &&
         memcmp(buffer + ARM64_MAGIC_OFFSET, "ARM\x64", 4) == 0) {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 173f8f67f6e3e79c..7e7f49261a309012 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -857,6 +857,70 @@ ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
     return bytes;
 }
 
+// The Linux header magic number for a EFI PE/COFF
+// image targetting an unspecified architecture.
+#define LINUX_EFI_PE_MAGIC        "\xcd\x23\x82\x81"
+
+struct linux_efi_zboot_header {
+    uint8_t     msdos_magic[4];         // PE/COFF 'MZ' magic number
+    uint8_t     zimg[4];                // "zimg" for Linux EFI zboot images
+    uint32_t    payload_offset;         // LE offset to the compressed payload
+    uint32_t    payload_size;           // LE size of the compressed payload
+    uint8_t     reserved[8];
+    char        compression_type[32];   // Compression type, e.g., "gzip"
+    uint8_t     linux_magic[4];         // Linux header magic
+    uint32_t    pe_header_offset;       // LE offset to the PE header
+};
+
+/*
+ * Check whether *buffer points to a Linux EFI zboot image in memory.
+ *
+ * If it does, attempt to decompress it to a new buffer, and free the old one.
+ * If any of this fails, return an error to the caller.
+ *
+ * If the image is not a Linux EFI zboot image, do nothing and return success.
+ */
+int unpack_efi_zboot_image(uint8_t **buffer, int *size)
+{
+    const struct linux_efi_zboot_header *header;
+    uint8_t *data = NULL;
+    ssize_t bytes;
+
+    /* ignore if this is too small to be a EFI zboot image */
+    if (*size < sizeof(*header)) {
+        return 0;
+    }
+
+    header = (struct linux_efi_zboot_header *)*buffer;
+
+    /* ignore if this is not a Linux EFI zboot image */
+    if (memcmp(&header->zimg, "zimg", 4) != 0 ||
+        memcmp(&header->linux_magic, LINUX_EFI_PE_MAGIC, 4) != 0) {
+        return 0;
+    }
+
+    if (strncmp(header->compression_type, "gzip", 4) != 0) {
+        fprintf(stderr, "unable to handle EFI zboot image with \"%s\" compression\n",
+                header->compression_type);
+        return -1;
+    }
+
+    data = g_malloc(LOAD_IMAGE_MAX_GUNZIP_BYTES);
+    bytes = gunzip(data, LOAD_IMAGE_MAX_GUNZIP_BYTES,
+                   *buffer + le32_to_cpu(header->payload_offset),
+                   le32_to_cpu(header->payload_size));
+    if (bytes < 0) {
+        fprintf(stderr, "failed to decompress EFI zboot image\n");
+        g_free(data);
+        return -1;
+    }
+
+    g_free(*buffer);
+    *buffer = g_realloc(data, bytes);
+    *size = bytes;
+    return 0;
+}
+
 /*
  * Functions for reboot-persistent memory regions.
  *  - used for vga bios and option roms.
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 70248e0da77908c1..d1092c8bfbd903c7 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -86,6 +86,8 @@ ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
                                   uint8_t **buffer);
 ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
 
+int unpack_efi_zboot_image(uint8_t **buffer, int *size);
+
 #define ELF_LOAD_FAILED       -1
 #define ELF_LOAD_NOT_ELF      -2
 #define ELF_LOAD_WRONG_ARCH   -3
-- 
2.39.1

Re: [RFC PATCH] hw: arm: Support direct boot for Linux/arm64 EFI zboot images

[Peter Maydell]

On Thu, 23 Feb 2023 at 10:53, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Fedora 39 will ship its arm64 kernels in the new generic EFI zboot
> format, using gzip compression for the payload.
>
> For doing EFI boot in QEMU, this is completely transparent, as the
> firmware or bootloader will take care of this. However, for direct
> kernel boot without firmware, we will lose the ability to boot such
> distro kernels unless we deal with the new format directly.
>
> EFI zboot images contain metadata in the header regarding the placement
> of the compressed payload inside the image, and the type of compression
> used. This means we can wire up the existing gzip support without too
> much hassle, by parsing the header and grabbing the payload from inside
> the loaded zboot image.

Seems reasonable to me. Any particular reason for marking the
patch RFC ?

> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  hw/arm/boot.c       |  4 ++
>  hw/core/loader.c    | 64 ++++++++++++++++++++
>  include/hw/loader.h |  2 +
>  3 files changed, 70 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 3d7d11f782feb5da..dc10a0788227443e 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -924,6 +924,10 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
>          size = len;
>      }
>
> +    if (unpack_efi_zboot_image(&buffer, &size)) {
> +        return -1;
> +    }

It seems a bit odd that we will now accept a gzipped file, unzip
it and then look inside it for the EFI zboot image that tells us
to do a second unzip step. Is that intentional/useful?
If not, probably better to do something like "if this is an
EFI zboot image, load-and-decompress, otherwise if a plain gzipped
file, load-and-decompress, otherwise assume a raw file".

> +
>      /* check the arm64 magic header value -- very old kernels may not have it */
>      if (size > ARM64_MAGIC_OFFSET + 4 &&
>          memcmp(buffer + ARM64_MAGIC_OFFSET, "ARM\x64", 4) == 0) {
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 173f8f67f6e3e79c..7e7f49261a309012 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -857,6 +857,70 @@ ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
>      return bytes;
>  }

I assume there's a spec somewhere that defines the file format;
this would be a good place for a comment giving a reference to it
(URL, document name, etc).

> +// The Linux header magic number for a EFI PE/COFF
> +// image targetting an unspecified architecture.
> +#define LINUX_EFI_PE_MAGIC        "\xcd\x23\x82\x81"
> +
> +struct linux_efi_zboot_header {
> +    uint8_t     msdos_magic[4];         // PE/COFF 'MZ' magic number
> +    uint8_t     zimg[4];                // "zimg" for Linux EFI zboot images
> +    uint32_t    payload_offset;         // LE offset to the compressed payload
> +    uint32_t    payload_size;           // LE size of the compressed payload
> +    uint8_t     reserved[8];
> +    char        compression_type[32];   // Compression type, e.g., "gzip"
> +    uint8_t     linux_magic[4];         // Linux header magic
> +    uint32_t    pe_header_offset;       // LE offset to the PE header
> +};

QEMU coding standard doesn't use '//' style comments.

> +
> +/*
> + * Check whether *buffer points to a Linux EFI zboot image in memory.
> + *
> + * If it does, attempt to decompress it to a new buffer, and free the old one.
> + * If any of this fails, return an error to the caller.
> + *
> + * If the image is not a Linux EFI zboot image, do nothing and return success.
> + */
> +int unpack_efi_zboot_image(uint8_t **buffer, int *size)
> +{
> +    const struct linux_efi_zboot_header *header;
> +    uint8_t *data = NULL;
> +    ssize_t bytes;
> +
> +    /* ignore if this is too small to be a EFI zboot image */
> +    if (*size < sizeof(*header)) {
> +        return 0;
> +    }
> +
> +    header = (struct linux_efi_zboot_header *)*buffer;

This isn't valid, because *buffer might not be properly aligned.
You can deal with that by defining your uint32_t fields to be uint8_t[]
and accessing the contents via ldl_le_p().

> +
> +    /* ignore if this is not a Linux EFI zboot image */
> +    if (memcmp(&header->zimg, "zimg", 4) != 0 ||
> +        memcmp(&header->linux_magic, LINUX_EFI_PE_MAGIC, 4) != 0) {

Do we not care about checking the msdos_magic[] ?

> +        return 0;
> +    }
> +
> +    if (strncmp(header->compression_type, "gzip", 4) != 0) {

Is this field supposed to be NUL-terminated? If I am not confused
about strncmp(), I think this is comparing only the first 4
characters and so would match both "gzip" and "gzip-but-not-really".

> +        fprintf(stderr, "unable to handle EFI zboot image with \"%s\" compression\n",
> +                header->compression_type);

This assumes the field is NUL-terminated and will do something
silly if fed a file where it is not.

> +        return -1;
> +    }
> +
> +    data = g_malloc(LOAD_IMAGE_MAX_GUNZIP_BYTES);
> +    bytes = gunzip(data, LOAD_IMAGE_MAX_GUNZIP_BYTES,
> +                   *buffer + le32_to_cpu(header->payload_offset),
> +                   le32_to_cpu(header->payload_size));

I think we should bounds-check that the payload offset and size
are actually all within the input buffer first.

> +    if (bytes < 0) {
> +        fprintf(stderr, "failed to decompress EFI zboot image\n");
> +        g_free(data);
> +        return -1;
> +    }
> +
> +    g_free(*buffer);
> +    *buffer = g_realloc(data, bytes);
> +    *size = bytes;
> +    return 0;
> +}
> +
>  /*
>   * Functions for reboot-persistent memory regions.
>   *  - used for vga bios and option roms.
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 70248e0da77908c1..d1092c8bfbd903c7 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -86,6 +86,8 @@ ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
>                                    uint8_t **buffer);
>  ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>
> +int unpack_efi_zboot_image(uint8_t **buffer, int *size);
> +

New global functions should have a doc-comment format comment
describing them in the header file. (This is one of those areas where
we have a bunch of legacy code that doesn't conform to our ideals and
are trying to gradually ratchet up by imposing a standard on new
contributions.)

thanks
-- PMM

Re: [RFC PATCH] hw: arm: Support direct boot for Linux/arm64 EFI zboot images

[Ard Biesheuvel]

On Fri, 3 Mar 2023 at 15:25, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 23 Feb 2023 at 10:53, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Fedora 39 will ship its arm64 kernels in the new generic EFI zboot
> > format, using gzip compression for the payload.
> >
> > For doing EFI boot in QEMU, this is completely transparent, as the
> > firmware or bootloader will take care of this. However, for direct
> > kernel boot without firmware, we will lose the ability to boot such
> > distro kernels unless we deal with the new format directly.
> >
> > EFI zboot images contain metadata in the header regarding the placement
> > of the compressed payload inside the image, and the type of compression
> > used. This means we can wire up the existing gzip support without too
> > much hassle, by parsing the header and grabbing the payload from inside
> > the loaded zboot image.
>
> Seems reasonable to me. Any particular reason for marking the
> patch RFC ?
>

Nothing except for the fact that I contribute so rarely that I may
have violated some coding style rules inadvertently.

> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Alex Bennée <alex.bennee@linaro.org>
> > Cc: Richard Henderson <richard.henderson@linaro.org>
> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  hw/arm/boot.c       |  4 ++
> >  hw/core/loader.c    | 64 ++++++++++++++++++++
> >  include/hw/loader.h |  2 +
> >  3 files changed, 70 insertions(+)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index 3d7d11f782feb5da..dc10a0788227443e 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -924,6 +924,10 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
> >          size = len;
> >      }
> >
> > +    if (unpack_efi_zboot_image(&buffer, &size)) {
> > +        return -1;
> > +    }
>
> It seems a bit odd that we will now accept a gzipped file, unzip
> it and then look inside it for the EFI zboot image that tells us
> to do a second unzip step. Is that intentional/useful?

No and no.

> If not, probably better to do something like "if this is an
> EFI zboot image, load-and-decompress, otherwise if a plain gzipped
> file, load-and-decompress, otherwise assume a raw file".
>
> > +
> >      /* check the arm64 magic header value -- very old kernels may not have it */
> >      if (size > ARM64_MAGIC_OFFSET + 4 &&
> >          memcmp(buffer + ARM64_MAGIC_OFFSET, "ARM\x64", 4) == 0) {
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 173f8f67f6e3e79c..7e7f49261a309012 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -857,6 +857,70 @@ ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
> >      return bytes;
> >  }
>
> I assume there's a spec somewhere that defines the file format;
> this would be a good place for a comment giving a reference to it
> (URL, document name, etc).
>

It is de facto defined by the Linux kernel's EFI stub - I can link to
the right file here

> > +// The Linux header magic number for a EFI PE/COFF
> > +// image targetting an unspecified architecture.
> > +#define LINUX_EFI_PE_MAGIC        "\xcd\x23\x82\x81"
> > +
> > +struct linux_efi_zboot_header {
> > +    uint8_t     msdos_magic[4];         // PE/COFF 'MZ' magic number
> > +    uint8_t     zimg[4];                // "zimg" for Linux EFI zboot images
> > +    uint32_t    payload_offset;         // LE offset to the compressed payload
> > +    uint32_t    payload_size;           // LE size of the compressed payload
> > +    uint8_t     reserved[8];
> > +    char        compression_type[32];   // Compression type, e.g., "gzip"
> > +    uint8_t     linux_magic[4];         // Linux header magic
> > +    uint32_t    pe_header_offset;       // LE offset to the PE header
> > +};
>
> QEMU coding standard doesn't use '//' style comments.
>

OK

> > +
> > +/*
> > + * Check whether *buffer points to a Linux EFI zboot image in memory.
> > + *
> > + * If it does, attempt to decompress it to a new buffer, and free the old one.
> > + * If any of this fails, return an error to the caller.
> > + *
> > + * If the image is not a Linux EFI zboot image, do nothing and return success.
> > + */
> > +int unpack_efi_zboot_image(uint8_t **buffer, int *size)
> > +{
> > +    const struct linux_efi_zboot_header *header;
> > +    uint8_t *data = NULL;
> > +    ssize_t bytes;
> > +
> > +    /* ignore if this is too small to be a EFI zboot image */
> > +    if (*size < sizeof(*header)) {
> > +        return 0;
> > +    }
> > +
> > +    header = (struct linux_efi_zboot_header *)*buffer;
>
> This isn't valid, because *buffer might not be properly aligned.
> You can deal with that by defining your uint32_t fields to be uint8_t[]
> and accessing the contents via ldl_le_p().
>

OK

> > +
> > +    /* ignore if this is not a Linux EFI zboot image */
> > +    if (memcmp(&header->zimg, "zimg", 4) != 0 ||
> > +        memcmp(&header->linux_magic, LINUX_EFI_PE_MAGIC, 4) != 0) {
>
> Do we not care about checking the msdos_magic[] ?
>

We could check it as well, sure, although LINUX_EFI_PE_MAGIC implies
that it is a PE/COFF image so it would be more of a sanity check.

> > +        return 0;
> > +    }
> > +
> > +    if (strncmp(header->compression_type, "gzip", 4) != 0) {
>
> Is this field supposed to be NUL-terminated? If I am not confused
> about strncmp(), I think this is comparing only the first 4
> characters and so would match both "gzip" and "gzip-but-not-really".
>

We've already checked the header size, so I suppose a plain strcmp()
is fine, with one argument being a literal NUL terminated string.

> > +        fprintf(stderr, "unable to handle EFI zboot image with \"%s\" compression\n",
> > +                header->compression_type);
>
> This assumes the field is NUL-terminated and will do something
> silly if fed a file where it is not.
>

It should be, but I can limit it in the printf() i suppose - will fix.

> > +        return -1;
> > +    }
> > +
> > +    data = g_malloc(LOAD_IMAGE_MAX_GUNZIP_BYTES);
> > +    bytes = gunzip(data, LOAD_IMAGE_MAX_GUNZIP_BYTES,
> > +                   *buffer + le32_to_cpu(header->payload_offset),
> > +                   le32_to_cpu(header->payload_size));
>
> I think we should bounds-check that the payload offset and size
> are actually all within the input buffer first.
>

Good point.

> > +    if (bytes < 0) {
> > +        fprintf(stderr, "failed to decompress EFI zboot image\n");
> > +        g_free(data);
> > +        return -1;
> > +    }
> > +
> > +    g_free(*buffer);
> > +    *buffer = g_realloc(data, bytes);
> > +    *size = bytes;
> > +    return 0;
> > +}
> > +
> >  /*
> >   * Functions for reboot-persistent memory regions.
> >   *  - used for vga bios and option roms.
> > diff --git a/include/hw/loader.h b/include/hw/loader.h
> > index 70248e0da77908c1..d1092c8bfbd903c7 100644
> > --- a/include/hw/loader.h
> > +++ b/include/hw/loader.h
> > @@ -86,6 +86,8 @@ ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> >                                    uint8_t **buffer);
> >  ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
> >
> > +int unpack_efi_zboot_image(uint8_t **buffer, int *size);
> > +
>
> New global functions should have a doc-comment format comment
> describing them in the header file. (This is one of those areas where
> we have a bunch of legacy code that doesn't conform to our ideals and
> are trying to gradually ratchet up by imposing a standard on new
> contributions.)
>

Fair enough.