[PATCH 2/3] arm64: efi: Ensure efi_create_mapping() does not map overlapping regions

[Catalin Marinas]

On Mon, Jun 06, 2016 at 11:43:01AM +0200, Ard Biesheuvel wrote:
> On 31 May 2016 at 17:14, Catalin Marinas <catalin.marinas at arm.com> wrote:
> > Since the EFI page size is 4KB, it is possible for a !4KB page kernel to
> > align an EFI runtime map boundaries in a way that they can overlap
> > within the same page. This requires the current create_pgd_mapping()
> > code to be able to split existing larger mappings when an overlapping
> > region needs to be mapped.
> >
> > With this patch, efi_create_mapping() scans the EFI memory map for
> > overlapping regions and trims the length of the current map to avoid a
> > large block mapping and subsequent split.
> 
> I remember the discussion, but IIRC this was about failure to do
> break-before-make in general rather than a problem with splitting in
> particular.

In this instance, we don't have break-before-make issue since the pgd
used for EFI runtime services is not active. So it's just about
splitting a larger block and simplifying the arch code.

> So first of all, I would like to point out that this failure to do
> break-before-make is not a problem in practice, given that the EFI
> page tables are not live until much later. The reason we want to fix
> this is because the EFI page tables routines are shared with the
> swapper, where it /is/ a concern, and there is a desire to make those
> routines more strict when it comes to architectural rules regarding
> page table manipulation.

Exactly.

> Also, the reference to block mappings on !4K pages kernels is a bit
> misleading here: such block mappings are at least 32 MB in size, and
> we never map runtime code or data regions that big.

Jeremy has some patches in flight to use the contiguous bit on such
mappings. With 64KB x 32 pages, we get a 2MB block. I want to avoid code
splitting such contiguous block as well (which with the contiguous bit
means going back and clearing it).

> > Signed-off-by: Catalin Marinas <catalin.marinas at arm.com>
> > ---
> >  arch/arm64/kernel/efi.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> > index 78f52488f9ff..0d5753c31c7f 100644
> > --- a/arch/arm64/kernel/efi.c
> > +++ b/arch/arm64/kernel/efi.c
> > @@ -62,10 +62,26 @@ struct screen_info screen_info __section(.data);
> >  int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
> >  {
> >         pteval_t prot_val = create_mapping_protection(md);
> > +       phys_addr_t length = md->num_pages << EFI_PAGE_SHIFT;
> > +       efi_memory_desc_t *next = md;
> >
> > -       create_pgd_mapping(mm, md->phys_addr, md->virt_addr,
> > -                          md->num_pages << EFI_PAGE_SHIFT,
> > -                          __pgprot(prot_val | PTE_NG));
> > +       /*
> > +        * Search for the next EFI runtime map and check for any overlap with
> > +        * the current map when aligned to PAGE_SIZE. In such case, defer
> > +        * mapping the end of the current range until the next
> > +        * efi_create_mapping() call.
> > +        */
> > +       for_each_efi_memory_desc_continue(next) {
> > +               if (!(next->attribute & EFI_MEMORY_RUNTIME))
> > +                       continue;
> > +               if (next->phys_addr < PAGE_ALIGN(md->phys_addr + length))
> > +                       length -= (md->phys_addr + length) & ~PAGE_MASK;
> 
> This looks fishy. We could have more than two runtime regions sharing
> a 64 KB page frame, for instance, and the middle regions will have
> unaligned start and end addresses, and sizes smaller than 64 KB. This
> subtraction may wrap in that case, producing a bogus length.

I don't think I get it. This subtraction is meant to reduce the length
of the current md so that it is page aligned, deferring the mapping of
the last page to the next efi_create_mapping() call. Note that there is
a "break" just after the hunk you quoted, so we only care about the next
runtime map.

-- 
Catalin