[Qemu-devel] [PATCH v4] aarch64: Allow -kernel option to take a gzip-compressed

[Qemu-devel] [PATCH v4] aarch64: Allow -kernel option to take a gzip-compressed

[Richard W.M. Jones]

No change compared to v3.  I just rebased the patch on top of current
HEAD and ensured that it still works.

Any comments at all on this approach?  It's the last patch I need to
make libguestfs work on aarch64 ...

Rich.

[Qemu-devel] [PATCH v4] aarch64: Allow -kernel option to take a gzip-compressed kernel.

[Richard W.M. Jones]

On aarch64 it is the bootloader's job to uncompress the kernel.  UEFI
and u-boot bootloaders do this automatically when the kernel is
gzip-compressed.

However the qemu -kernel option does not do this.  The following
command does not work:

  qemu-system-aarch64 [...] -kernel /boot/vmlinuz

because it tries to execute the gzip-compressed data.

This commit lets gzip-compressed kernels be uncompressed
transparently.

Currently this is only done when emulating aarch64.

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 hw/arm/boot.c       |  9 +++++++++
 hw/core/loader.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/loader.h |  1 +
 3 files changed, 53 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 3d1f4a2..1d541db 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -444,6 +444,7 @@ static void do_cpu_reset(void *opaque)
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 {
     CPUState *cs = CPU(cpu);
+    int allow_compressed_kernels = 0;
     int kernel_size;
     int initrd_size;
     int is_linux = 0;
@@ -465,6 +466,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         primary_loader = bootloader_aarch64;
         kernel_load_offset = KERNEL64_LOAD_ADDR;
         elf_machine = EM_AARCH64;
+        allow_compressed_kernels = 1;
     } else {
         primary_loader = bootloader;
         kernel_load_offset = KERNEL_LOAD_ADDR;
@@ -510,6 +512,13 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         kernel_size = load_uimage(info->kernel_filename, &entry, NULL,
                                   &is_linux);
     }
+    /* On aarch64, it's the bootloader's job to uncompress the kernel. */
+    if (allow_compressed_kernels && kernel_size < 0) {
+        entry = info->loader_start + kernel_load_offset;
+        kernel_size = load_image_gzipped(info->kernel_filename, entry,
+                                         info->ram_size - kernel_load_offset);
+        is_linux = 1;
+    }
     if (kernel_size < 0) {
         entry = info->loader_start + kernel_load_offset;
         kernel_size = load_image_targphys(info->kernel_filename, entry,
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 2bf6b8f..1cbe733 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -577,6 +577,49 @@ int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
     return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK);
 }
 
+/* Load a gzip-compressed kernel (currently used by aarch64). */
+int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
+{
+    uint8_t *compressed_data = NULL;
+    uint8_t *data = NULL;
+    gsize len;
+    size_t max_bytes;
+    ssize_t bytes;
+    int ret = -1;
+
+    if (!g_file_get_contents(filename, (char **) &compressed_data, &len,
+                             NULL)) {
+        goto out;
+    }
+
+    /* Is it a gzip-compressed file? */
+    if (len < 2 ||
+        compressed_data[0] != '\x1f' ||
+        compressed_data[1] != '\x8b') {
+        goto out;
+    }
+
+    max_bytes = UBOOT_MAX_GUNZIP_BYTES;
+    if (max_bytes > max_sz) {
+        max_bytes = max_sz;
+    }
+    data = g_malloc(max_bytes);
+    bytes = gunzip(data, max_bytes, compressed_data, len);
+    if (bytes < 0) {
+        fprintf(stderr, "%s: unable to decompress gzipped kernel file\n",
+                filename);
+        goto out;
+    }
+
+    rom_add_blob_fixed(filename, data, bytes, addr);
+    ret = bytes;
+
+ out:
+    g_free(compressed_data);
+    g_free(data);
+    return ret;
+}
+
 /*
  * 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 796cbf9..00c9117 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -15,6 +15,7 @@ int get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */
 int load_image_targphys(const char *filename, hwaddr,
                         uint64_t max_sz);
+int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
 
 #define ELF_LOAD_FAILED       -1
 #define ELF_LOAD_NOT_ELF      -2
-- 
2.0.1

Re: [Qemu-devel] [PATCH v4] aarch64: Allow -kernel option to take a gzip-compressed kernel.

[Peter Crosthwaite]

On Sun, Aug 3, 2014 at 1:45 AM, Richard W.M. Jones <rjones@redhat.com> wrote:
> On aarch64 it is the bootloader's job to uncompress the kernel.  UEFI
> and u-boot bootloaders do this automatically when the kernel is
> gzip-compressed.
>
> However the qemu -kernel option does not do this.  The following
> command does not work:
>
>   qemu-system-aarch64 [...] -kernel /boot/vmlinuz
>
> because it tries to execute the gzip-compressed data.
>
> This commit lets gzip-compressed kernels be uncompressed
> transparently.
>
> Currently this is only done when emulating aarch64.
>
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  hw/arm/boot.c       |  9 +++++++++
>  hw/core/loader.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/loader.h |  1 +

There's two patches here, one adding the generic gzip loader (which
has wider review audience than just ARM people), and a second one
adding the AArch64 usage.

>  3 files changed, 53 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 3d1f4a2..1d541db 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -444,6 +444,7 @@ static void do_cpu_reset(void *opaque)
>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>  {
>      CPUState *cs = CPU(cpu);
> +    int allow_compressed_kernels = 0;
>      int kernel_size;
>      int initrd_size;
>      int is_linux = 0;
> @@ -465,6 +466,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>          primary_loader = bootloader_aarch64;
>          kernel_load_offset = KERNEL64_LOAD_ADDR;
>          elf_machine = EM_AARCH64;
> +        allow_compressed_kernels = 1;
>      } else {
>          primary_loader = bootloader;
>          kernel_load_offset = KERNEL_LOAD_ADDR;
> @@ -510,6 +512,13 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>          kernel_size = load_uimage(info->kernel_filename, &entry, NULL,
>                                    &is_linux);
>      }
> +    /* On aarch64, it's the bootloader's job to uncompress the kernel. */
> +    if (allow_compressed_kernels && kernel_size < 0) {
> +        entry = info->loader_start + kernel_load_offset;
> +        kernel_size = load_image_gzipped(info->kernel_filename, entry,
> +                                         info->ram_size - kernel_load_offset);
> +        is_linux = 1;
> +    }
>      if (kernel_size < 0) {
>          entry = info->loader_start + kernel_load_offset;
>          kernel_size = load_image_targphys(info->kernel_filename, entry,
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2bf6b8f..1cbe733 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -577,6 +577,49 @@ int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
>      return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK);
>  }
>
> +/* Load a gzip-compressed kernel (currently used by aarch64). */

No need to refer to "aarch64" in generic code (this comment is likely
to go stale).

> +int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
> +{
> +    uint8_t *compressed_data = NULL;
> +    uint8_t *data = NULL;
> +    gsize len;
> +    size_t max_bytes;
> +    ssize_t bytes;
> +    int ret = -1;
> +
> +    if (!g_file_get_contents(filename, (char **) &compressed_data, &len,
> +                             NULL)) {
> +        goto out;
> +    }
> +
> +    /* Is it a gzip-compressed file? */
> +    if (len < 2 ||
> +        compressed_data[0] != '\x1f' ||
> +        compressed_data[1] != '\x8b') {
> +        goto out;
> +    }
> +
> +    max_bytes = UBOOT_MAX_GUNZIP_BYTES;

Why does u-boot's maximum size limit apply here?

Regards,
Peter

> +    if (max_bytes > max_sz) {
> +        max_bytes = max_sz;
> +    }
> +    data = g_malloc(max_bytes);
> +    bytes = gunzip(data, max_bytes, compressed_data, len);
> +    if (bytes < 0) {
> +        fprintf(stderr, "%s: unable to decompress gzipped kernel file\n",
> +                filename);
> +        goto out;
> +    }
> +
> +    rom_add_blob_fixed(filename, data, bytes, addr);
> +    ret = bytes;
> +
> + out:
> +    g_free(compressed_data);
> +    g_free(data);
> +    return ret;
> +}
> +
>  /*
>   * 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 796cbf9..00c9117 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -15,6 +15,7 @@ int get_image_size(const char *filename);
>  int load_image(const char *filename, uint8_t *addr); /* deprecated */
>  int load_image_targphys(const char *filename, hwaddr,
>                          uint64_t max_sz);
> +int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>
>  #define ELF_LOAD_FAILED       -1
>  #define ELF_LOAD_NOT_ELF      -2
> --
> 2.0.1
>
>

Re: [Qemu-devel] [PATCH v4] aarch64: Allow -kernel option to take a gzip-compressed kernel.

[Richard W.M. Jones]

On Mon, Aug 04, 2014 at 09:05:39AM +1000, Peter Crosthwaite wrote:
> On Sun, Aug 3, 2014 at 1:45 AM, Richard W.M. Jones <rjones@redhat.com> wrote:
> > +    max_bytes = UBOOT_MAX_GUNZIP_BYTES;
> 
> Why does u-boot's maximum size limit apply here?

We need some maximum to prevent people uploading a kernel (perhaps
from an untrusted source) which is some sort of malicious gzip file
that expands to a huge size.

In this case the u-boot limit is 64 MB which is larger than most
possible kernels, so it seemed like a reasonable limit to choose.
You're right there is no connection to u-boot, except that both the
-kernel option and u-boot have similar concerns with maximum kernel
size, and presumably the u-boot limit is battle-tested.

I'll split the patch into two and send v5 soon.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH v4] aarch64: Allow -kernel option to take a gzip-compressed kernel.

[Peter Maydell]

On 4 August 2014 09:48, Richard W.M. Jones <rjones@redhat.com> wrote:
> On Mon, Aug 04, 2014 at 09:05:39AM +1000, Peter Crosthwaite wrote:
>> On Sun, Aug 3, 2014 at 1:45 AM, Richard W.M. Jones <rjones@redhat.com> wrote:
>> > +    max_bytes = UBOOT_MAX_GUNZIP_BYTES;
>>
>> Why does u-boot's maximum size limit apply here?
>
> We need some maximum to prevent people uploading a kernel (perhaps
> from an untrusted source) which is some sort of malicious gzip file
> that expands to a huge size.

If we care about malicious zipfiles we should probably fix the bits
in gunzip() which trust the gzip header more than they should...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4] aarch64: Allow -kernel option to take a gzip-compressed kernel.

[Peter Crosthwaite]

On Mon, Aug 4, 2014 at 6:48 PM, Richard W.M. Jones <rjones@redhat.com> wrote:
> On Mon, Aug 04, 2014 at 09:05:39AM +1000, Peter Crosthwaite wrote:
>> On Sun, Aug 3, 2014 at 1:45 AM, Richard W.M. Jones <rjones@redhat.com> wrote:
>> > +    max_bytes = UBOOT_MAX_GUNZIP_BYTES;
>>
>> Why does u-boot's maximum size limit apply here?
>
> We need some maximum to prevent people uploading a kernel (perhaps
> from an untrusted source) which is some sort of malicious gzip file
> that expands to a huge size.
>
> In this case the u-boot limit is 64 MB which is larger than most
> possible kernels, so it seemed like a reasonable limit to choose.

Ok. If you really do need this artificial limit then I think you
should just make your own macro with the same value.

Regards,
Peter

> You're right there is no connection to u-boot, except that both the
> -kernel option and u-boot have similar concerns with maximum kernel
> size, and presumably the u-boot limit is battle-tested.
>
> I'll split the patch into two and send v5 soon.
>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> libguestfs lets you edit virtual machines.  Supports shell scripting,
> bindings from many languages.  http://libguestfs.org
>