On Thu, 23 Feb 2023 at 10:53, Ard Biesheuvel <[email protected]> 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 <[email protected]> > Cc: Alex Bennée <[email protected]> > Cc: Richard Henderson <[email protected]> > Cc: Philippe Mathieu-Daudé <[email protected]> > Signed-off-by: Ard Biesheuvel <[email protected]> > --- > 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
