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.