| Message ID | 20170419171311.3243-4-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Properly map the FDT in the boot page table | expand |
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@arm.com> > --- > 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@kernel.org> > 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 >
Hi Stefano, On 19/04/2017 22:01, Stefano Stabellini wrote: > 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@arm.com> >> --- >> 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. Whilst I agree the first one is not that useful, this one will catch any change in the memory memory map that will not keep BOOT_FDT_VIRT_START 2MB aligned. > > >> 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: It will be useful later on and wanted to avoid too much code modification in later patch. > > 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? I blindly copied from the kernel. I can fix that. > > These are all minor stylistic nits, so > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > >> 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 >>
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); + 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); 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; + + 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); + 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 */
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@arm.com> --- 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(-)