[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH] hw: arm: Support direct boot for Linux/arm64 EFI zboot i
From: |
Peter Maydell |
Subject: |
Re: [RFC PATCH] hw: arm: Support direct boot for Linux/arm64 EFI zboot images |
Date: |
Fri, 3 Mar 2023 14:25:23 +0000 |
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,
Peter Maydell <=