[PATCH 1/2] arm64: mm: detect bad __create_mapping uses

[Mark Rutland]

On Mon, Nov 23, 2015 at 11:01:51AM +0000, Steve Capper wrote:
> On 20 November 2015 at 17:05, Mark Rutland <mark.rutland at arm.com> wrote:
> > On Fri, Nov 20, 2015 at 04:13:28PM +0000, Steve Capper wrote:
> >> On 20 November 2015 at 15:35, Mark Rutland <mark.rutland at arm.com> wrote:
> >> > If a caller of __create_mapping provides a PA and VA which have
> >> > different sub-page offsets, it is not clear which offset they expect to
> >> > apply to the mapping, and is indicative of a bad caller.
> >> >
> >> > Disallow calls with differing sub-page offsets, and WARN when they are
> >> > encountered, so that we can detect and fix such cases.
> >> >
> >> > Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> >> > Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> >> > Cc: Catalin Marinas <caralin.marinas at arm.com>
> >> > Cc: Laura Abbott <labbott at fedoraproject.org>
> >> > Cc: Steve Capper <steve.capper at linaro.org>
> >> > Cc: Will Deacon <will.deacon at arm.com>
> >> > ---
> >> >  arch/arm64/mm/mmu.c | 7 +++++++
> >> >  1 file changed, 7 insertions(+)
> >> >
> >> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> > index e3f563c..3b06afa 100644
> >> > --- a/arch/arm64/mm/mmu.c
> >> > +++ b/arch/arm64/mm/mmu.c
> >> > @@ -300,6 +300,13 @@ static void  __create_mapping(struct mm_struct *mm, pgd_t *pgd,
> >> >  {
> >> >         unsigned long addr, length, end, next;
> >> >
> >> > +       /*
> >> > +        * If the virtual and physical address don't have the same offset
> >> > +        * within a page, we cannot map the region as the caller expects.
> >> > +        */
> >> > +       if (WARN_ON((phys ^ virt) & ~PAGE_MASK))
> >> > +               return;
> >> > +
> >>
> >> Do we want any subpage offsets in our virt and phys?
> >
> > Yes.
> >
> > For instance, when mapping EFI regions (which exist at 4K granularity)
> > on a 64K page kernel, per [1]. The spec requires that any regions in the
> > same 64K page can be mapped using the same attributes, so it's fine to
> > map at 64K granularity.
> 
> Thanks Mark,
> I didn't know that was allowed, now I do :-).
> These two patches now make perfect sense.
> 
> >
> > We already try to cater for this in __create_mapping where we fix up the
> > base and size:
> >
> >         addr = virt & PAGE_MASK;
> >         length = PAGE_ALIGN(size + (virt & ~PAGE_MASK));
> >
> > I will add a middle paragraph to the commit message:
> >
> >         In some cases, the region we wish to map may validly have a
> >         sub-page offset in the physical and virtual adddresses. For
> >         example, EFI runtime regions have 4K granularity, yet may be
> >         mapped by a 64K page kernel. So long as the physical and virtual
> >         offsets are the same, the region will be mapped at the expected
> >         VAs.
> >
> > Does that sound OK?
> 
> s/adddresses/addresses/
> Otherwise that looks great, thanks.

Good spot. Fixed.

> >> If we have sub page bits in phys won't that start flipping on bits in
> >> the page table entries?
> >
> > In all cases where we use phys, we either shift phys right by PAGE_SHIFT
> > (in __populate_init_pte vi __phys_to_pfn) which clears those bits, or we
> > first check it is sufficiently aligned for that level of table (and
> > hence the low bits are all zero). Patch 2 fixes a bug related to the
> > latter case.
> >
> >> What about something like:
> >> if (WARN_ON((phys | virt) & ~PAGE_MASK))
> >
> > Per the above, that would break the EFI code in [1].
> 
> Agreed.
> 
> I was toying with suggesting we warn for any phys/virt bit set below
> bit #12 (i.e. anything smaller than 4K alignment), that would probably
> just increase complexity with little appreciable gain though.

I'm not sure either way, to be honest. We might want to use it in an
ioremap-like manner to map small objects.

For instance, we use create_mapping to map the DTB in fixmap_remap_fdt,
and the DTB itself is only guaranteed to be 8-byte aligned. However,
fixmap_remap_fdt has to handle the sub-page offset explicitly either
way, either adding it to the fixmap slot's virtual address or
subtracting it from the physical base when it makes the call.

> For both patches (with the extra paragraph above added to the commit log):
> Reviewed-by: Steve Capper <steve.capper at linaro.org>

I take it you mean with the paragraph added to this patch, not to both
patches?

Thanks,
Mark.