[Qemu-devel] [PATCH v2 0/4] hw/arm/boot: handle large Images more gracefully

[Qemu-devel] [PATCH v2 0/4] hw/arm/boot: handle large Images more gracefully

[Peter Maydell]

This patchset attempts to fix https://bugs.launchpad.net/qemu/+bug/1823998
which reports that we don't handle kernels larger than 128MB
correctly, because we allow the initrd to be placed over the
tail end of the kernel. AArch64 kernel Image files (since v3.17)
report the total size they require (including any BSS area that
isn't in the Image itself), so we can use that to be sure we
place the initrd sufficiently far into the RAM.

Patches 1 and 2 are new since v1; patches 3 and 4 are the old
patches 1 and 2 (and are basically unchanged since v1).

Patches 1 and 2 in this series are new. Patch 1 fixes bugs
in the existing code where we were assuming that we could
treat info->ram_size as the address of the end of RAM, which
isn't true if the RAM doesn't start at address 0. (This
generally went unnoticed thanks to the magic of unsigned integer
underflow turning end-start calculations into very large max_size
values for load_ramdisk_as() and friends.)
Patch 2 adds some explicit checks that we don't try to put things
entirely off the end of RAM (which avoids those accidental
underflows).
Patch 3 in this series adjusts our "where do we put the initrd"
heuristic so that it always places it at least after whatever
our best guess at the kernel size is. (This might still not
be right for images like self-decompressing 32-bit kernels, where
there's no way to know how big the kernel will be after
decompression.)
Patch 4 makes load_aarch64_image() return the
kernel size as indicated in the Image file header, so that for
the specific case of AArch64 Image files we will definitely not
put the initrd on top of them.

thanks
-- PMM

Peter Maydell (4):
  hw/arm/boot: Don't assume RAM starts at address zero
  hw/arm/boot: Diagnose layouts that put initrd or DTB off the end of
    RAM
  hw/arm/boot: Avoid placing the initrd on top of the kernel
  hw/arm/boot: Honour image size field in AArch64 Image format kernels

 hw/arm/boot.c | 83 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 62 insertions(+), 21 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/4] hw/arm/boot: Don't assume RAM starts at address zero

[Peter Maydell]

In the Arm kernel/initrd loading code, in some places we make the
incorrect assumption that info->ram_size can be treated as the
address of the end of RAM, as for instance when we calculate the
available space for the initrd using "info->ram_size - info->initrd_start".
This is wrong, because many Arm boards (including "virt") specify
a non-zero info->loader_start to indicate that their RAM area
starts at a non-zero physical address.

Correct the places which make this incorrect assumption.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index a830655e1af..0bb9a7d5b5c 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -976,6 +976,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
     int elf_machine;
     hwaddr entry;
     static const ARMInsnFixup *primary_loader;
+    uint64_t ram_end = info->loader_start + info->ram_size;
 
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         primary_loader = bootloader_aarch64;
@@ -1047,8 +1048,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
         /* 32-bit ARM */
         entry = info->loader_start + KERNEL_LOAD_ADDR;
         kernel_size = load_image_targphys_as(info->kernel_filename, entry,
-                                             info->ram_size - KERNEL_LOAD_ADDR,
-                                             as);
+                                             ram_end - KERNEL_LOAD_ADDR, as);
         is_linux = 1;
     }
     if (kernel_size < 0) {
@@ -1062,12 +1062,11 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
         if (info->initrd_filename) {
             initrd_size = load_ramdisk_as(info->initrd_filename,
                                           info->initrd_start,
-                                          info->ram_size - info->initrd_start,
-                                          as);
+                                          ram_end - info->initrd_start, as);
             if (initrd_size < 0) {
                 initrd_size = load_image_targphys_as(info->initrd_filename,
                                                      info->initrd_start,
-                                                     info->ram_size -
+                                                     ram_end -
                                                      info->initrd_start,
                                                      as);
             }
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/4] hw/arm/boot: Diagnose layouts that put initrd or DTB off the end of RAM

[Peter Maydell]

We calculate the locations in memory where we want to put the
initrd and the DTB based on the size of the kernel, since they
come after it. Add some explicit checks that these aren't off the
end of RAM entirely.

(At the moment the way we calculate the initrd_start means that
it can't ever be off the end of RAM, but that will change with
the next commit.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 0bb9a7d5b5c..935be3b92a5 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1055,11 +1055,25 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
         error_report("could not load kernel '%s'", info->kernel_filename);
         exit(1);
     }
+
+    if (kernel_size > info->ram_size) {
+        error_report("kernel '%s' is too large to fit in RAM "
+                     "(kernel size %d, RAM size %" PRId64 ")",
+                     info->kernel_filename, kernel_size, info->ram_size);
+        exit(1);
+    }
+
     info->entry = entry;
     if (is_linux) {
         uint32_t fixupcontext[FIXUP_MAX];
 
         if (info->initrd_filename) {
+
+            if (info->initrd_start >= ram_end) {
+                error_report("not enough space after kernel to load initrd");
+                exit(1);
+            }
+
             initrd_size = load_ramdisk_as(info->initrd_filename,
                                           info->initrd_start,
                                           ram_end - info->initrd_start, as);
@@ -1075,6 +1089,11 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
                              info->initrd_filename);
                 exit(1);
             }
+            if (info->initrd_start + initrd_size > info->ram_size) {
+                error_report("could not load initrd '%s': "
+                             "too big to fit into RAM after the kernel",
+                             info->initrd_filename);
+            }
         } else {
             initrd_size = 0;
         }
@@ -1110,6 +1129,10 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
             /* Place the DTB after the initrd in memory with alignment. */
             info->dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
                                            align);
+            if (info->dtb_start >= ram_end) {
+                error_report("Not enough space for DTB after kernel/initrd");
+                exit(1);
+            }
             fixupcontext[FIXUP_ARGPTR_LO] = info->dtb_start;
             fixupcontext[FIXUP_ARGPTR_HI] = info->dtb_start >> 32;
         } else {
-- 
2.20.1

[Qemu-devel] [PATCH v2 3/4] hw/arm/boot: Avoid placing the initrd on top of the kernel

[Peter Maydell]

We currently put the initrd at the smaller of:
 * 128MB into RAM
 * halfway into the RAM
(with the dtb following it).

However for large kernels this might mean that the kernel
overlaps the initrd. For some kinds of kernel (self-decompressing
32-bit kernels, and ELF images with a BSS section at the end)
we don't know the exact size, but even there we have a
minimum size. Put the initrd at least further into RAM than
that. For image formats that can give us an exact kernel size, this
will mean that we definitely avoid overlaying kernel and initrd.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 935be3b92a5..e441393fdf5 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
     if (info->nb_cpus == 0)
         info->nb_cpus = 1;
 
-    /*
-     * We want to put the initrd far enough into RAM that when the
-     * kernel is uncompressed it will not clobber the initrd. However
-     * on boards without much RAM we must ensure that we still leave
-     * enough room for a decent sized initrd, and on boards with large
-     * amounts of RAM we must avoid the initrd being so far up in RAM
-     * that it is outside lowmem and inaccessible to the kernel.
-     * So for boards with less  than 256MB of RAM we put the initrd
-     * halfway into RAM, and for boards with 256MB of RAM or more we put
-     * the initrd at 128MB.
-     */
-    info->initrd_start = info->loader_start +
-        MIN(info->ram_size / 2, 128 * 1024 * 1024);
-
     /* Assume that raw images are linux kernels, and ELF images are not.  */
     kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
                                &elf_high_addr, elf_machine, as);
@@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
     }
 
     info->entry = entry;
+
+    /*
+     * We want to put the initrd far enough into RAM that when the
+     * kernel is uncompressed it will not clobber the initrd. However
+     * on boards without much RAM we must ensure that we still leave
+     * enough room for a decent sized initrd, and on boards with large
+     * amounts of RAM we must avoid the initrd being so far up in RAM
+     * that it is outside lowmem and inaccessible to the kernel.
+     * So for boards with less  than 256MB of RAM we put the initrd
+     * halfway into RAM, and for boards with 256MB of RAM or more we put
+     * the initrd at 128MB.
+     * We also refuse to put the initrd somewhere that will definitely
+     * overlay the kernel we just loaded, though for kernel formats which
+     * don't tell us their exact size (eg self-decompressing 32-bit kernels)
+     * we might still make a bad choice here.
+     */
+    info->initrd_start = info->loader_start +
+        MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
+    info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start);
+
     if (is_linux) {
         uint32_t fixupcontext[FIXUP_MAX];
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 4/4] hw/arm/boot: Honour image size field in AArch64 Image format kernels

[Peter Maydell]

Since Linux v3.17, the kernel's Image header includes a field image_size,
which gives the total size of the kernel including unpopulated data
sections such as the BSS). If this is present, then return it from
load_aarch64_image() as the true size of the kernel rather than
just using the size of the Image file itself. This allows the code
which calculates where to put the initrd to avoid putting it in
the kernel's BSS area.

This means that we should be able to reliably load kernel images
which are larger than 128MB without accidentally putting the
initrd or dtb in locations that clash with the kernel itself.

Fixes: https://bugs.launchpad.net/qemu/+bug/1823998
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/arm/boot.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index e441393fdf5..fc6f37ba6cf 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -910,6 +910,7 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
                                    hwaddr *entry, AddressSpace *as)
 {
     hwaddr kernel_load_offset = KERNEL64_LOAD_ADDR;
+    uint64_t kernel_size = 0;
     uint8_t *buffer;
     int size;
 
@@ -937,7 +938,10 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
          * is only valid if the image_size is non-zero.
          */
         memcpy(&hdrvals, buffer + ARM64_TEXT_OFFSET_OFFSET, sizeof(hdrvals));
-        if (hdrvals[1] != 0) {
+
+        kernel_size = le64_to_cpu(hdrvals[1]);
+
+        if (kernel_size != 0) {
             kernel_load_offset = le64_to_cpu(hdrvals[0]);
 
             /*
@@ -955,12 +959,21 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
         }
     }
 
+    /*
+     * Kernels before v3.17 don't populate the image_size field, and
+     * raw images have no header. For those our best guess at the size
+     * is the size of the Image file itself.
+     */
+    if (kernel_size == 0) {
+        kernel_size = size;
+    }
+
     *entry = mem_base + kernel_load_offset;
     rom_add_blob_fixed_as(filename, buffer, size, *entry, as);
 
     g_free(buffer);
 
-    return size;
+    return kernel_size;
 }
 
 static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 0/4] hw/arm/boot: handle large Images more gracefully

[Peter Maydell]

Ping for code review, please?

thanks
-- PMM

On Thu, 16 May 2019 at 15:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>
>
> This patchset attempts to fix https://bugs.launchpad.net/qemu/+bug/1823998
> which reports that we don't handle kernels larger than 128MB
> correctly, because we allow the initrd to be placed over the
> tail end of the kernel. AArch64 kernel Image files (since v3.17)
> report the total size they require (including any BSS area that
> isn't in the Image itself), so we can use that to be sure we
> place the initrd sufficiently far into the RAM.
>
> Patches 1 and 2 are new since v1; patches 3 and 4 are the old
> patches 1 and 2 (and are basically unchanged since v1).
>
> Patches 1 and 2 in this series are new. Patch 1 fixes bugs
> in the existing code where we were assuming that we could
> treat info->ram_size as the address of the end of RAM, which
> isn't true if the RAM doesn't start at address 0. (This
> generally went unnoticed thanks to the magic of unsigned integer
> underflow turning end-start calculations into very large max_size
> values for load_ramdisk_as() and friends.)
> Patch 2 adds some explicit checks that we don't try to put things
> entirely off the end of RAM (which avoids those accidental
> underflows).
> Patch 3 in this series adjusts our "where do we put the initrd"
> heuristic so that it always places it at least after whatever
> our best guess at the kernel size is. (This might still not
> be right for images like self-decompressing 32-bit kernels, where
> there's no way to know how big the kernel will be after
> decompression.)
> Patch 4 makes load_aarch64_image() return the
> kernel size as indicated in the Image file header, so that for
> the specific case of AArch64 Image files we will definitely not
> put the initrd on top of them.
>
> thanks
> -- PMM
>
> Peter Maydell (4):
>   hw/arm/boot: Don't assume RAM starts at address zero
>   hw/arm/boot: Diagnose layouts that put initrd or DTB off the end of
>     RAM
>   hw/arm/boot: Avoid placing the initrd on top of the kernel
>   hw/arm/boot: Honour image size field in AArch64 Image format kernels
>
>  hw/arm/boot.c | 83 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 62 insertions(+), 21 deletions(-)
>
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH v2 0/4] hw/arm/boot: handle large Images more gracefully

[Mark Rutland]

Hi Peter,

Sorry for the delay in replying to this...

On Thu, May 16, 2019 at 03:47:29PM +0100, Peter Maydell wrote:
> 
> This patchset attempts to fix https://bugs.launchpad.net/qemu/+bug/1823998
> which reports that we don't handle kernels larger than 128MB
> correctly, because we allow the initrd to be placed over the
> tail end of the kernel. AArch64 kernel Image files (since v3.17)
> report the total size they require (including any BSS area that
> isn't in the Image itself), so we can use that to be sure we
> place the initrd sufficiently far into the RAM.
> 
> Patches 1 and 2 are new since v1; patches 3 and 4 are the old
> patches 1 and 2 (and are basically unchanged since v1).
> 
> Patches 1 and 2 in this series are new. Patch 1 fixes bugs
> in the existing code where we were assuming that we could
> treat info->ram_size as the address of the end of RAM, which
> isn't true if the RAM doesn't start at address 0. (This
> generally went unnoticed thanks to the magic of unsigned integer
> underflow turning end-start calculations into very large max_size
> values for load_ramdisk_as() and friends.)
> Patch 2 adds some explicit checks that we don't try to put things
> entirely off the end of RAM (which avoids those accidental
> underflows).
> Patch 3 in this series adjusts our "where do we put the initrd"
> heuristic so that it always places it at least after whatever
> our best guess at the kernel size is. (This might still not
> be right for images like self-decompressing 32-bit kernels, where
> there's no way to know how big the kernel will be after
> decompression.)
> Patch 4 makes load_aarch64_image() return the
> kernel size as indicated in the Image file header, so that for
> the specific case of AArch64 Image files we will definitely not
> put the initrd on top of them.

With all 4 patches applied, I'm able to boot kernels with large BSS
segments (~128M, ~512M, and ~1G), and I get sensible warnings when they
are impossible to boot, e.g.

# 124M of RAM
[mark@gravadlaks:~/repro]% ./vmboot.sh ~/Image.test-128M
qemu-system-aarch64: kernel '/home/mark/Image.test-128M' is too large to fit in RAM (kernel size 155500544, RAM size 130023424)

# 150M of RAM
[mark@gravadlaks:~/repro]% ./vmboot.sh ~/Image.test-128M
qemu-system-aarch64: Not enough space for DTB after kernel/initrd

So feel free to add:

Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks for putting this together; this is _really_ useful for my testing
setup, and the warnings above are likely to save people a lot of
head-scratching in future. It would be great to see this merged. :)

Thanks,
Mark.

Re: [Qemu-devel] [PATCH v2 0/4] hw/arm/boot: handle large Images more gracefully

[Philippe Mathieu-Daudé]

Cc'ing Drew and Eric

On 6/7/19 3:07 PM, Peter Maydell wrote:
> Ping for code review, please?
> 
> thanks
> -- PMM
> 
> On Thu, 16 May 2019 at 15:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>>
>> This patchset attempts to fix https://bugs.launchpad.net/qemu/+bug/1823998
>> which reports that we don't handle kernels larger than 128MB
>> correctly, because we allow the initrd to be placed over the
>> tail end of the kernel. AArch64 kernel Image files (since v3.17)
>> report the total size they require (including any BSS area that
>> isn't in the Image itself), so we can use that to be sure we
>> place the initrd sufficiently far into the RAM.
>>
>> Patches 1 and 2 are new since v1; patches 3 and 4 are the old
>> patches 1 and 2 (and are basically unchanged since v1).
>>
>> Patches 1 and 2 in this series are new. Patch 1 fixes bugs
>> in the existing code where we were assuming that we could
>> treat info->ram_size as the address of the end of RAM, which
>> isn't true if the RAM doesn't start at address 0. (This
>> generally went unnoticed thanks to the magic of unsigned integer
>> underflow turning end-start calculations into very large max_size
>> values for load_ramdisk_as() and friends.)
>> Patch 2 adds some explicit checks that we don't try to put things
>> entirely off the end of RAM (which avoids those accidental
>> underflows).
>> Patch 3 in this series adjusts our "where do we put the initrd"
>> heuristic so that it always places it at least after whatever
>> our best guess at the kernel size is. (This might still not
>> be right for images like self-decompressing 32-bit kernels, where
>> there's no way to know how big the kernel will be after
>> decompression.)
>> Patch 4 makes load_aarch64_image() return the
>> kernel size as indicated in the Image file header, so that for
>> the specific case of AArch64 Image files we will definitely not
>> put the initrd on top of them.
>>
>> thanks
>> -- PMM
>>
>> Peter Maydell (4):
>>   hw/arm/boot: Don't assume RAM starts at address zero
>>   hw/arm/boot: Diagnose layouts that put initrd or DTB off the end of
>>     RAM
>>   hw/arm/boot: Avoid placing the initrd on top of the kernel
>>   hw/arm/boot: Honour image size field in AArch64 Image format kernels
>>
>>  hw/arm/boot.c | 83 ++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 62 insertions(+), 21 deletions(-)
>>
>> --
>> 2.20.1
>>
> 

Re: [Qemu-devel] [PATCH v2 1/4] hw/arm/boot: Don't assume RAM starts at address zero

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> In the Arm kernel/initrd loading code, in some places we make the
> incorrect assumption that info->ram_size can be treated as the
> address of the end of RAM, as for instance when we calculate the
> available space for the initrd using "info->ram_size - info->initrd_start".
> This is wrong, because many Arm boards (including "virt") specify
> a non-zero info->loader_start to indicate that their RAM area
> starts at a non-zero physical address.
>
> Correct the places which make this incorrect assumption.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  hw/arm/boot.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index a830655e1af..0bb9a7d5b5c 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -976,6 +976,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>      int elf_machine;
>      hwaddr entry;
>      static const ARMInsnFixup *primary_loader;
> +    uint64_t ram_end = info->loader_start + info->ram_size;
>
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>          primary_loader = bootloader_aarch64;
> @@ -1047,8 +1048,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>          /* 32-bit ARM */
>          entry = info->loader_start + KERNEL_LOAD_ADDR;
>          kernel_size = load_image_targphys_as(info->kernel_filename, entry,
> -                                             info->ram_size - KERNEL_LOAD_ADDR,
> -                                             as);
> +                                             ram_end - KERNEL_LOAD_ADDR, as);
>          is_linux = 1;
>      }
>      if (kernel_size < 0) {
> @@ -1062,12 +1062,11 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>          if (info->initrd_filename) {
>              initrd_size = load_ramdisk_as(info->initrd_filename,
>                                            info->initrd_start,
> -                                          info->ram_size - info->initrd_start,
> -                                          as);
> +                                          ram_end - info->initrd_start, as);
>              if (initrd_size < 0) {
>                  initrd_size = load_image_targphys_as(info->initrd_filename,
>                                                       info->initrd_start,
> -                                                     info->ram_size -
> +                                                     ram_end -
>                                                       info->initrd_start,
>                                                       as);
>              }


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 2/4] hw/arm/boot: Diagnose layouts that put initrd or DTB off the end of RAM

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> We calculate the locations in memory where we want to put the
> initrd and the DTB based on the size of the kernel, since they
> come after it. Add some explicit checks that these aren't off the
> end of RAM entirely.
>
> (At the moment the way we calculate the initrd_start means that
> it can't ever be off the end of RAM, but that will change with
> the next commit.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  hw/arm/boot.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 0bb9a7d5b5c..935be3b92a5 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -1055,11 +1055,25 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>          error_report("could not load kernel '%s'", info->kernel_filename);
>          exit(1);
>      }
> +
> +    if (kernel_size > info->ram_size) {
> +        error_report("kernel '%s' is too large to fit in RAM "
> +                     "(kernel size %d, RAM size %" PRId64 ")",
> +                     info->kernel_filename, kernel_size, info->ram_size);
> +        exit(1);
> +    }
> +
>      info->entry = entry;
>      if (is_linux) {
>          uint32_t fixupcontext[FIXUP_MAX];
>
>          if (info->initrd_filename) {
> +
> +            if (info->initrd_start >= ram_end) {
> +                error_report("not enough space after kernel to load initrd");
> +                exit(1);
> +            }
> +
>              initrd_size = load_ramdisk_as(info->initrd_filename,
>                                            info->initrd_start,
>                                            ram_end - info->initrd_start, as);
> @@ -1075,6 +1089,11 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>                               info->initrd_filename);
>                  exit(1);
>              }
> +            if (info->initrd_start + initrd_size > info->ram_size) {
> +                error_report("could not load initrd '%s': "
> +                             "too big to fit into RAM after the kernel",
> +                             info->initrd_filename);
> +            }
>          } else {
>              initrd_size = 0;
>          }
> @@ -1110,6 +1129,10 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>              /* Place the DTB after the initrd in memory with alignment. */
>              info->dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
>                                             align);
> +            if (info->dtb_start >= ram_end) {
> +                error_report("Not enough space for DTB after kernel/initrd");
> +                exit(1);
> +            }
>              fixupcontext[FIXUP_ARGPTR_LO] = info->dtb_start;
>              fixupcontext[FIXUP_ARGPTR_HI] = info->dtb_start >> 32;
>          } else {


--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 3/4] hw/arm/boot: Avoid placing the initrd on top of the kernel

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> We currently put the initrd at the smaller of:
>  * 128MB into RAM
>  * halfway into the RAM
> (with the dtb following it).
>
> However for large kernels this might mean that the kernel
> overlaps the initrd. For some kinds of kernel (self-decompressing
> 32-bit kernels, and ELF images with a BSS section at the end)
> we don't know the exact size, but even there we have a
> minimum size. Put the initrd at least further into RAM than
> that. For image formats that can give us an exact kernel size, this
> will mean that we definitely avoid overlaying kernel and initrd.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


> ---
>  hw/arm/boot.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 935be3b92a5..e441393fdf5 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>      if (info->nb_cpus == 0)
>          info->nb_cpus = 1;
>
> -    /*
> -     * We want to put the initrd far enough into RAM that when the
> -     * kernel is uncompressed it will not clobber the initrd. However
> -     * on boards without much RAM we must ensure that we still leave
> -     * enough room for a decent sized initrd, and on boards with large
> -     * amounts of RAM we must avoid the initrd being so far up in RAM
> -     * that it is outside lowmem and inaccessible to the kernel.
> -     * So for boards with less  than 256MB of RAM we put the initrd
> -     * halfway into RAM, and for boards with 256MB of RAM or more we put
> -     * the initrd at 128MB.
> -     */
> -    info->initrd_start = info->loader_start +
> -        MIN(info->ram_size / 2, 128 * 1024 * 1024);
> -
>      /* Assume that raw images are linux kernels, and ELF images are not.  */
>      kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
>                                 &elf_high_addr, elf_machine, as);
> @@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>      }
>
>      info->entry = entry;
> +
> +    /*
> +     * We want to put the initrd far enough into RAM that when the
> +     * kernel is uncompressed it will not clobber the initrd. However
> +     * on boards without much RAM we must ensure that we still leave
> +     * enough room for a decent sized initrd, and on boards with large
> +     * amounts of RAM we must avoid the initrd being so far up in RAM
> +     * that it is outside lowmem and inaccessible to the kernel.
> +     * So for boards with less  than 256MB of RAM we put the initrd
> +     * halfway into RAM, and for boards with 256MB of RAM or more we put
> +     * the initrd at 128MB.
> +     * We also refuse to put the initrd somewhere that will definitely
> +     * overlay the kernel we just loaded, though for kernel formats which
> +     * don't tell us their exact size (eg self-decompressing 32-bit kernels)
> +     * we might still make a bad choice here.
> +     */
> +    info->initrd_start = info->loader_start +
> +        MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
> +    info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start);
> +
>      if (is_linux) {
>          uint32_t fixupcontext[FIXUP_MAX];


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 4/4] hw/arm/boot: Honour image size field in AArch64 Image format kernels

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Since Linux v3.17, the kernel's Image header includes a field image_size,
> which gives the total size of the kernel including unpopulated data
> sections such as the BSS). If this is present, then return it from
> load_aarch64_image() as the true size of the kernel rather than
> just using the size of the Image file itself. This allows the code
> which calculates where to put the initrd to avoid putting it in
> the kernel's BSS area.
>
> This means that we should be able to reliably load kernel images
> which are larger than 128MB without accidentally putting the
> initrd or dtb in locations that clash with the kernel itself.
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1823998
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  hw/arm/boot.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index e441393fdf5..fc6f37ba6cf 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -910,6 +910,7 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
>                                     hwaddr *entry, AddressSpace *as)
>  {
>      hwaddr kernel_load_offset = KERNEL64_LOAD_ADDR;
> +    uint64_t kernel_size = 0;
>      uint8_t *buffer;
>      int size;
>
> @@ -937,7 +938,10 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
>           * is only valid if the image_size is non-zero.
>           */
>          memcpy(&hdrvals, buffer + ARM64_TEXT_OFFSET_OFFSET, sizeof(hdrvals));
> -        if (hdrvals[1] != 0) {
> +
> +        kernel_size = le64_to_cpu(hdrvals[1]);
> +
> +        if (kernel_size != 0) {
>              kernel_load_offset = le64_to_cpu(hdrvals[0]);
>
>              /*
> @@ -955,12 +959,21 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base,
>          }
>      }
>
> +    /*
> +     * Kernels before v3.17 don't populate the image_size field, and
> +     * raw images have no header. For those our best guess at the size
> +     * is the size of the Image file itself.
> +     */
> +    if (kernel_size == 0) {
> +        kernel_size = size;
> +    }
> +
>      *entry = mem_base + kernel_load_offset;
>      rom_add_blob_fixed_as(filename, buffer, size, *entry, as);
>
>      g_free(buffer);
>
> -    return size;
> +    return kernel_size;
>  }
>
>  static void arm_setup_direct_kernel_boot(ARMCPU *cpu,


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 3/4] hw/arm/boot: Avoid placing the initrd on top of the kernel

[Mark Rutland]

Hi Peter,

I've just been testing on QEMU v4.1.0-rc1, and found a case where the
DTB overlapped the end of the kernel, and I think there's a bug in this
patch -- explanation below.

On Thu, May 16, 2019 at 03:47:32PM +0100, Peter Maydell wrote:
> We currently put the initrd at the smaller of:
>  * 128MB into RAM
>  * halfway into the RAM
> (with the dtb following it).
> 
> However for large kernels this might mean that the kernel
> overlaps the initrd. For some kinds of kernel (self-decompressing
> 32-bit kernels, and ELF images with a BSS section at the end)
> we don't know the exact size, but even there we have a
> minimum size. Put the initrd at least further into RAM than
> that. For image formats that can give us an exact kernel size, this
> will mean that we definitely avoid overlaying kernel and initrd.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/boot.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 935be3b92a5..e441393fdf5 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>      if (info->nb_cpus == 0)
>          info->nb_cpus = 1;
>  
> -    /*
> -     * We want to put the initrd far enough into RAM that when the
> -     * kernel is uncompressed it will not clobber the initrd. However
> -     * on boards without much RAM we must ensure that we still leave
> -     * enough room for a decent sized initrd, and on boards with large
> -     * amounts of RAM we must avoid the initrd being so far up in RAM
> -     * that it is outside lowmem and inaccessible to the kernel.
> -     * So for boards with less  than 256MB of RAM we put the initrd
> -     * halfway into RAM, and for boards with 256MB of RAM or more we put
> -     * the initrd at 128MB.
> -     */
> -    info->initrd_start = info->loader_start +
> -        MIN(info->ram_size / 2, 128 * 1024 * 1024);
> -
>      /* Assume that raw images are linux kernels, and ELF images are not.  */
>      kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
>                                 &elf_high_addr, elf_machine, as);
> @@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>      }
>  
>      info->entry = entry;

Note: this is the start of the kernel image...

> +
> +    /*
> +     * We want to put the initrd far enough into RAM that when the
> +     * kernel is uncompressed it will not clobber the initrd. However
> +     * on boards without much RAM we must ensure that we still leave
> +     * enough room for a decent sized initrd, and on boards with large
> +     * amounts of RAM we must avoid the initrd being so far up in RAM
> +     * that it is outside lowmem and inaccessible to the kernel.
> +     * So for boards with less  than 256MB of RAM we put the initrd
> +     * halfway into RAM, and for boards with 256MB of RAM or more we put
> +     * the initrd at 128MB.
> +     * We also refuse to put the initrd somewhere that will definitely
> +     * overlay the kernel we just loaded, though for kernel formats which
> +     * don't tell us their exact size (eg self-decompressing 32-bit kernels)
> +     * we might still make a bad choice here.
> +     */
> +    info->initrd_start = info->loader_start +
> +        MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);

... but here we add kernel_size to the start of the loader, which is
below the kernel. Should that be info->entry?

I've seen this trigger a case where:

* The kernel's image_size is 0x0a7a8000
* The kernel was loaded at   0x40080000
* The end of the kernel is   0x4A828000
* The DTB was loaded at      0x4a800000

... and the kernel is unable to find a usable DTB.

Thanks,
Mark.

> +    info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start);
> +
>      if (is_linux) {
>          uint32_t fixupcontext[FIXUP_MAX];
>  
> -- 
> 2.20.1
> 

Re: [Qemu-devel] [PATCH v2 3/4] hw/arm/boot: Avoid placing the initrd on top of the kernel

[Peter Maydell]

On Fri, 19 Jul 2019 at 17:47, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Peter,
>
> I've just been testing on QEMU v4.1.0-rc1, and found a case where the
> DTB overlapped the end of the kernel, and I think there's a bug in this
> patch -- explanation below.
>
> On Thu, May 16, 2019 at 03:47:32PM +0100, Peter Maydell wrote:
> > We currently put the initrd at the smaller of:
> >  * 128MB into RAM
> >  * halfway into the RAM
> > (with the dtb following it).
> >
> > However for large kernels this might mean that the kernel
> > overlaps the initrd. For some kinds of kernel (self-decompressing
> > 32-bit kernels, and ELF images with a BSS section at the end)
> > we don't know the exact size, but even there we have a
> > minimum size. Put the initrd at least further into RAM than
> > that. For image formats that can give us an exact kernel size, this
> > will mean that we definitely avoid overlaying kernel and initrd.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/arm/boot.c | 34 ++++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index 935be3b92a5..e441393fdf5 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> >      if (info->nb_cpus == 0)
> >          info->nb_cpus = 1;
> >
> > -    /*
> > -     * We want to put the initrd far enough into RAM that when the
> > -     * kernel is uncompressed it will not clobber the initrd. However
> > -     * on boards without much RAM we must ensure that we still leave
> > -     * enough room for a decent sized initrd, and on boards with large
> > -     * amounts of RAM we must avoid the initrd being so far up in RAM
> > -     * that it is outside lowmem and inaccessible to the kernel.
> > -     * So for boards with less  than 256MB of RAM we put the initrd
> > -     * halfway into RAM, and for boards with 256MB of RAM or more we put
> > -     * the initrd at 128MB.
> > -     */
> > -    info->initrd_start = info->loader_start +
> > -        MIN(info->ram_size / 2, 128 * 1024 * 1024);
> > -
> >      /* Assume that raw images are linux kernels, and ELF images are not.  */
> >      kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
> >                                 &elf_high_addr, elf_machine, as);
> > @@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> >      }
> >
> >      info->entry = entry;
>
> Note: this is the start of the kernel image...

It's the entry point, which isn't quite the same thing as
the start of the image (if we just loaded an ELF file then
'entry' is whatever the ELF file said the entrypoint is, which
could be a long way into the image).

> > +
> > +    /*
> > +     * We want to put the initrd far enough into RAM that when the
> > +     * kernel is uncompressed it will not clobber the initrd. However
> > +     * on boards without much RAM we must ensure that we still leave
> > +     * enough room for a decent sized initrd, and on boards with large
> > +     * amounts of RAM we must avoid the initrd being so far up in RAM
> > +     * that it is outside lowmem and inaccessible to the kernel.
> > +     * So for boards with less  than 256MB of RAM we put the initrd
> > +     * halfway into RAM, and for boards with 256MB of RAM or more we put
> > +     * the initrd at 128MB.
> > +     * We also refuse to put the initrd somewhere that will definitely
> > +     * overlay the kernel we just loaded, though for kernel formats which
> > +     * don't tell us their exact size (eg self-decompressing 32-bit kernels)
> > +     * we might still make a bad choice here.
> > +     */
> > +    info->initrd_start = info->loader_start +
> > +        MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
>
> ... but here we add kernel_size to the start of the loader, which is
> below the kernel. Should that be info->entry?

loader_start here really means "base of RAM". I think what
we want here is something like

  info->initrd_start = info->loader_start + MIN(info->ram_size / 2,
128 * 1024 * 1024);
  info->initrd_start = MAX(info->initrd_start, kernel_end);

where kernel_end is just past whatever the highest addr of the kernel
is, which is not something that's totally trivial to calculate
with the variables we have to hand at this point.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/4] hw/arm/boot: Avoid placing the initrd on top of the kernel

[Mark Rutland]

On Mon, Jul 22, 2019 at 12:59:01PM +0100, Peter Maydell wrote:
> On Fri, 19 Jul 2019 at 17:47, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Hi Peter,
> >
> > I've just been testing on QEMU v4.1.0-rc1, and found a case where the
> > DTB overlapped the end of the kernel, and I think there's a bug in this
> > patch -- explanation below.
> >
> > On Thu, May 16, 2019 at 03:47:32PM +0100, Peter Maydell wrote:
> > > We currently put the initrd at the smaller of:
> > >  * 128MB into RAM
> > >  * halfway into the RAM
> > > (with the dtb following it).
> > >
> > > However for large kernels this might mean that the kernel
> > > overlaps the initrd. For some kinds of kernel (self-decompressing
> > > 32-bit kernels, and ELF images with a BSS section at the end)
> > > we don't know the exact size, but even there we have a
> > > minimum size. Put the initrd at least further into RAM than
> > > that. For image formats that can give us an exact kernel size, this
> > > will mean that we definitely avoid overlaying kernel and initrd.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >  hw/arm/boot.c | 34 ++++++++++++++++++++--------------
> > >  1 file changed, 20 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > > index 935be3b92a5..e441393fdf5 100644
> > > --- a/hw/arm/boot.c
> > > +++ b/hw/arm/boot.c
> > > @@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> > >      if (info->nb_cpus == 0)
> > >          info->nb_cpus = 1;
> > >
> > > -    /*
> > > -     * We want to put the initrd far enough into RAM that when the
> > > -     * kernel is uncompressed it will not clobber the initrd. However
> > > -     * on boards without much RAM we must ensure that we still leave
> > > -     * enough room for a decent sized initrd, and on boards with large
> > > -     * amounts of RAM we must avoid the initrd being so far up in RAM
> > > -     * that it is outside lowmem and inaccessible to the kernel.
> > > -     * So for boards with less  than 256MB of RAM we put the initrd
> > > -     * halfway into RAM, and for boards with 256MB of RAM or more we put
> > > -     * the initrd at 128MB.
> > > -     */
> > > -    info->initrd_start = info->loader_start +
> > > -        MIN(info->ram_size / 2, 128 * 1024 * 1024);
> > > -
> > >      /* Assume that raw images are linux kernels, and ELF images are not.  */
> > >      kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
> > >                                 &elf_high_addr, elf_machine, as);
> > > @@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> > >      }
> > >
> > >      info->entry = entry;
> >
> > Note: this is the start of the kernel image...
> 
> It's the entry point, which isn't quite the same thing as
> the start of the image (if we just loaded an ELF file then
> 'entry' is whatever the ELF file said the entrypoint is, which
> could be a long way into the image).

Ah, I see; thanks for correcting me!

> > > +    /*
> > > +     * We want to put the initrd far enough into RAM that when the
> > > +     * kernel is uncompressed it will not clobber the initrd. However
> > > +     * on boards without much RAM we must ensure that we still leave
> > > +     * enough room for a decent sized initrd, and on boards with large
> > > +     * amounts of RAM we must avoid the initrd being so far up in RAM
> > > +     * that it is outside lowmem and inaccessible to the kernel.
> > > +     * So for boards with less  than 256MB of RAM we put the initrd
> > > +     * halfway into RAM, and for boards with 256MB of RAM or more we put
> > > +     * the initrd at 128MB.
> > > +     * We also refuse to put the initrd somewhere that will definitely
> > > +     * overlay the kernel we just loaded, though for kernel formats which
> > > +     * don't tell us their exact size (eg self-decompressing 32-bit kernels)
> > > +     * we might still make a bad choice here.
> > > +     */
> > > +    info->initrd_start = info->loader_start +
> > > +        MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
> >
> > ... but here we add kernel_size to the start of the loader, which is
> > below the kernel. Should that be info->entry?
> 
> loader_start here really means "base of RAM". I think what
> we want here is something like
> 
>   info->initrd_start = info->loader_start + MIN(info->ram_size / 2,
> 128 * 1024 * 1024);
>   info->initrd_start = MAX(info->initrd_start, kernel_end);

From what I understand, I can't see a way of breaking that, so it looks
good to me.

> where kernel_end is just past whatever the highest addr of the kernel
> is, which is not something that's totally trivial to calculate
> with the variables we have to hand at this point.

Sure; I assume you might need to drop that into arm_boot_info.

As previously, I'm happy to test patches for this. At the moment I have
a local hack relying on info->entry, and I understand this isn't
correct.

Thanks,
Mark.