Re: [Xen-devel] [PATCH for-4.9 3/4] xen/arm: Check if the FDT passed by the bootloader is valid

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 19 Apr 2017, Julien Grall wrote:
> There is currently no sanity check on the FDT passed by the bootloader.
> Whilst they are stricly not necessary, it will avoid us to spend hours
> to try to find out why it does not work.
> 
> >From the booting documentation for AArch32 [1] and AArch64 [2] must :
>     - be placed on 8-byte boundary
>     - not exceed 2MB (only on AArch64)
> 
> Even if AArch32 does not seem to limit the size, Xen is not currently
> able to support more the 2MB FDT. It is better to crash rather with a nice
> error message than claiming we are supporting any size of FDT.
> 
> The checks are mostly borrowed from the Linux code (see fixmap_remap_fdt
> in arch/arm64/mm/mmu.c).
> 
> [1] Section 2 in linux/Documentation/arm64/booting.txt
> [2] Section 4b in linux/Documentation/arm/Booting
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/mm.c           | 29 ++++++++++++++++++++++++++++-
>  xen/arch/arm/setup.c        |  6 ++++++
>  xen/include/asm-arm/setup.h |  3 +++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0d076489c6..53d36e2ce2 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -39,6 +39,8 @@
>  #include <xsm/xsm.h>
>  #include <xen/pfn.h>
>  #include <xen/sizes.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <asm/setup.h>
>  
>  static void __init create_mappings(lpae_t *second,
>                                     unsigned long virt_offset,
> @@ -447,11 +449,36 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>  {
>      /* We are using 2MB superpage for mapping the FDT */
>      paddr_t base_paddr = fdt_paddr & SECOND_MASK;
> +    paddr_t offset;
> +    void *fdt_virt;
> +
> +    /*
> +     * Check whether the physical FDT address is set and meets the minimum
> +     * alignment requirement. Since we are relying on MIN_FDT_ALIGN to be at
> +     * least 8 bytes so that we always access the magic and size fields
> +     * of the FDT header after mapping the first chunk, double check if
> +     * that is indeed the case.
> +     */
> +    BUILD_BUG_ON(MIN_FDT_ALIGN < 8);

I think that this BUILD_BUG_ON is overkill, a comment would be enough


> +    if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN )
> +        return NULL;
> +
> +    /* The FDT is mapped using 2MB superpage */
> +    BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);

Same here.


>      create_mappings(boot_second, BOOT_FDT_VIRT_START, 
> paddr_to_pfn(base_paddr),
>                      SZ_2M >> PAGE_SHIFT, SZ_2M);
>  
> -    return (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE);
> +    offset = fdt_paddr % SECOND_SIZE;
> +    fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;

offset is useless, you can just:

      fdt_virt = (void *)BOOT_FDT_VIRT_START + (fdt_paddr % SECOND_SIZE);


> +    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> +        return NULL;
> +
> +    if ( fdt_totalsize(fdt_virt) > MAX_FDT_SIZE )
> +        return NULL;
> +
> +    return fdt_virt;
>  }
>  
>  void __init remove_early_mappings(void)
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 986398970f..8f72f31fb5 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -725,6 +725,12 @@ void __init start_xen(unsigned long boot_phys_offset,
>      smp_clear_cpu_maps();
>  
>      device_tree_flattened = early_fdt_map(fdt_paddr);
> +    if ( !device_tree_flattened )
> +        panic("Invalid device tree blob at physical address %#lx\n"
> +              "The DTB must be 8-byte aligned and must not exceed 2 MB in 
> size\n"
> +              "\nPlease check your bootloader.",
> +              fdt_paddr);

Strange, why did you place the "\n" at the beginning of the line?

These are all minor stylistic nits, so

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


>      fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
>  
>      cmdline = boot_fdt_cmdline(device_tree_flattened);
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 7c761851d2..7ff2c34dab 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -3,6 +3,9 @@
>  
>  #include <public/version.h>
>  
> +#define MIN_FDT_ALIGN 8
> +#define MAX_FDT_SIZE SZ_2M
> +
>  #define NR_MEM_BANKS 64
>  
>  #define MAX_MODULES 5 /* Current maximum useful modules */
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel