Re: [Qemu-devel] [PATCH v2 2/4] hw/arm/boot: pass an address limit to and return size from load_dtb()

[Peter Maydell]

On 10 September 2014 11:59, Ard Biesheuvel <address@hidden> wrote:
> Add an address limit input parameter to load_dtb() so that we can
> tell it how much memory the dtb is allowed to consume. If the dtb
> doesn't fit, return 0, otherwise return the actual size of the
> loaded dtb, or -1 on error.
>
> Signed-off-by: Ard Biesheuvel <address@hidden>
> ---
>  hw/arm/boot.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 50eca931e1a4..014fab347b09 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -312,7 +312,8 @@ static void set_kernel_args_old(const struct 
> arm_boot_info *info)
>      }
>  }
>
> -static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
> +static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> +                    hwaddr addr_limit)
>  {

This patch looks good, but since you're going to respin the series
anyway we could use a doc comment before the function describing the
semantics of the return value. If you do that you can
add my Reviewed-by: tag.

>      void *fdt = NULL;
>      int size, rc;
> @@ -341,6 +342,15 @@ static int load_dtb(hwaddr addr, const struct 
> arm_boot_info *binfo)
>          }
>      }
>
> +    if (addr_limit > addr && size > (addr_limit - addr)) {
> +        /* We have been given a non-zero address limit and we have exceeded
> +         * it. Whether this is constitues a failure is up to the caller to

"constitutes"

> +         * decide, so just return 0 as size, i.e., no error.
> +         */
> +        g_free(fdt);
> +        return 0;
> +    }
> +
>      acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
>      scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
>      if (acells == 0 || scells == 0) {
> @@ -403,7 +413,7 @@ static int load_dtb(hwaddr addr, const struct 
> arm_boot_info *binfo)
>
>      g_free(fdt);
>
> -    return 0;
> +    return size;
>
>  fail:
>      g_free(fdt);
> @@ -572,7 +582,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
> *info)
>               */
>              hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + 
> initrd_size,
>                                               4096);
> -            if (load_dtb(dtb_start, info)) {
> +            if (load_dtb(dtb_start, info, 0) < 0) {
>                  exit(1);
>              }
>              fixupcontext[FIXUP_ARGPTR] = dtb_start;

-- PMM