On Mon, Aug 01, 2016 at 11:50:39AM +0200, Ard Biesheuvel wrote: > On 1 August 2016 at 11:42, zijun_hu <zijun_hu@xxxxxxxx> wrote: > > From 07b9216ec3494515e7a6c41e0333eb8782427db3 Mon Sep 17 00:00:00 2001 > > From: zijun_hu <zijun_hu@xxxxxxx> > > Date: Mon, 1 Aug 2016 17:04:59 +0800 > > Subject: [PATCH] arm64: fix address fault during mapping fdt region > > > > fdt_check_header() accesses other fileds of fdt header but > > the first 8 bytes such as version; so accessing unmapped > > address fault happens if fdt region locates below align > > boundary nearly during mapping fdt region, or expressed as > > (offset + sizeof(struct fdt_header)) > SWAPPER_BLOCK_SIZE > > > > fdt header size at least is mapped in order to avoid the issue > > > > Signed-off-by: zijun_hu <zijun_hu@xxxxxxx> > > --- > > arch/arm64/mm/mmu.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > > index 0f85a46..0d72b71 100644 > > --- a/arch/arm64/mm/mmu.c > > +++ b/arch/arm64/mm/mmu.c > > @@ -744,6 +744,7 @@ void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot) > > const u64 dt_virt_base = __fix_to_virt(FIX_FDT); > > int offset; > > void *dt_virt; > > + int dt_header_map_size; > > > > /* > > * Check whether the physical FDT address is set and meets the minimum > > @@ -774,9 +775,18 @@ void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot) > > offset = dt_phys % SWAPPER_BLOCK_SIZE; > > dt_virt = (void *)dt_virt_base + offset; > > > > + /* > > + * fdt_check_header() maybe access any field of fdt header not > > + * the first 8 bytes only, so map fdt header size at least for > > + * checking fdt header without address fault more portably > > + */ > > + BUILD_BUG_ON(sizeof(struct fdt_header) > SWAPPER_BLOCK_SIZE); > > + dt_header_map_size = round_up(offset + sizeof(struct fdt_header), > > + SWAPPER_BLOCK_SIZE); > > + > > /* map the first chunk so we can read the size from the header */ > > create_mapping_noalloc(round_down(dt_phys, SWAPPER_BLOCK_SIZE), > > - dt_virt_base, SWAPPER_BLOCK_SIZE, prot); > > + dt_virt_base, dt_header_map_size, prot); > > > > if (fdt_check_header(dt_virt) != 0) > > return NULL; > > @@ -785,7 +795,7 @@ void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot) > > if (*size > MAX_FDT_SIZE) > > return NULL; > > > > - if (offset + *size > SWAPPER_BLOCK_SIZE) > > + if (offset + *size > dt_header_map_size) > > create_mapping_noalloc(round_down(dt_phys, SWAPPER_BLOCK_SIZE), dt_virt_base, > > round_up(offset + *size, SWAPPER_BLOCK_SIZE), prot); > > > > Couldn't we simply do this instead? > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 0f85a46c3e18..e8d3b04a2b57 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -778,7 +778,7 @@ void *__init __fixmap_remap_fdt(phys_addr_t > dt_phys, int *size, pgprot_t prot) > create_mapping_noalloc(round_down(dt_phys, SWAPPER_BLOCK_SIZE), > dt_virt_base, SWAPPER_BLOCK_SIZE, prot); > > - if (fdt_check_header(dt_virt) != 0) > + if (fdt_magic(dt_virt) != FDT_MAGIC) > return NULL; > > *size = fdt_totalsize(dt_virt); > > We are simply looking for a size field. The OF code will call > fdt_check_header() again, so anything that is checked there in > addition to the magic field will still be checked. That looks much nicer to me, and deferring the header check also looks fine. If you add a comment above the fdt_magic() check, pointing out why we can only safely access the magic and totalsize fields (and thus can't call fdt_check_header() yet): Acked-by: Mark Rutland <mark.rutland@xxxxxxx> Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html