[PATCH] arm64: efi: correctly align vaddr for runtime maps

[PATCH] arm64: efi: correctly align vaddr for runtime maps

[Mark Rutland]

The kernel may use a page granularity of 4K, 16K, or 64K depending on
configuration.

When mapping EFI runtime regions, we use memrange_efi_to_native to round
the physical base address of a region down to a granule-aligned
boundary, and round the size up to a granule-aligned boundary. However,
we fail to similarly round the virtual base address down to a
granule-aligned boundary.

The virtual base address may be up to PAGE_SIZE - 4K above what it
should be, and in create_pgd_mapping, we may erroneously map an
additional page at the end of any region which does not have a
granule-aligned virtual base address.

Depending on the memory map, this page may be in a region we are not
intended/permitted to map, or may clash with a different region that we
wich to map.

Prevent this issue by rounding the virtual base address down to the
kernel page granularity, matching what we do for the physical base
address.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/efi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

I spotted this by playing with Will's break-before-make checker [1], which
detected an erroneously created PTE being overwritten with a different output
address.

It looks like the VA bug was introduced in commit f3cdfd239da56a4c ("arm64/efi:
move SetVirtualAddressMap() to UEFI stub").

Prior to commit 60305db9884515ca ("arm64/efi: move virtmap init to early
initcall") so manual fixup is required, but the logic fix is the same.

Mark.

[1] https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/commit/?h=aarch64/devel&id=372f39220ad35fa39a75419f2221ffeb6ffd78d3

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index de46b50..7855b69 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -225,7 +225,7 @@ static bool __init efi_virtmap_init(void)
 	efi_memory_desc_t *md;
 
 	for_each_efi_memory_desc(&memmap, md) {
-		u64 paddr, npages, size;
+		u64 paddr, vaddr, npages, size;
 		pgprot_t prot;
 
 		if (!(md->attribute & EFI_MEMORY_RUNTIME))
@@ -237,6 +237,8 @@ static bool __init efi_virtmap_init(void)
 		npages = md->num_pages;
 		memrange_efi_to_native(&paddr, &npages);
 		size = npages << PAGE_SHIFT;
+		vaddr = md->virt_addr;
+		vaddr &= PAGE_MASK;
 
 		pr_info("  EFI remap 0x%016llx => %p\n",
 			md->phys_addr, (void *)md->virt_addr);
@@ -254,7 +256,7 @@ static bool __init efi_virtmap_init(void)
 		else
 			prot = PAGE_KERNEL;
 
-		create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
+		create_pgd_mapping(&efi_mm, paddr, vaddr, size, prot);
 	}
 	return true;
 }
-- 
1.9.1

[PATCH] arm64: efi: correctly align vaddr for runtime maps

[Ard Biesheuvel]

On 19 November 2015 at 18:37, Mark Rutland <mark.rutland@arm.com> wrote:
> The kernel may use a page granularity of 4K, 16K, or 64K depending on
> configuration.
>
> When mapping EFI runtime regions, we use memrange_efi_to_native to round
> the physical base address of a region down to a granule-aligned
> boundary, and round the size up to a granule-aligned boundary. However,
> we fail to similarly round the virtual base address down to a
> granule-aligned boundary.
>

Actually, __create_mapping() (which is called by create_pgd_mapping())
does the following

"""
static void  __create_mapping(struct mm_struct *mm, pgd_t *pgd,
                                    phys_addr_t phys, unsigned long virt,
                                    phys_addr_t size, pgprot_t prot,
                                    void *(*alloc)(unsigned long size))
{
        unsigned long addr, length, end, next;

        addr = virt & PAGE_MASK;
        length = PAGE_ALIGN(size + (virt & ~PAGE_MASK));
"""

so it does the rounding of the virtual address for us, but we are
rounding up the length twice.
I'd rather simply get rid of memrange_efi_to_native() instead, as it
is obviously redundant.

> The virtual base address may be up to PAGE_SIZE - 4K above what it
> should be, and in create_pgd_mapping, we may erroneously map an
> additional page at the end of any region which does not have a
> granule-aligned virtual base address.
>
> Depending on the memory map, this page may be in a region we are not
> intended/permitted to map, or may clash with a different region that we
> wich to map.
>
> Prevent this issue by rounding the virtual base address down to the
> kernel page granularity, matching what we do for the physical base
> address.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/efi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> I spotted this by playing with Will's break-before-make checker [1], which
> detected an erroneously created PTE being overwritten with a different output
> address.
>
> It looks like the VA bug was introduced in commit f3cdfd239da56a4c ("arm64/efi:
> move SetVirtualAddressMap() to UEFI stub").
>
> Prior to commit 60305db9884515ca ("arm64/efi: move virtmap init to early
> initcall") so manual fixup is required, but the logic fix is the same.
>

I don't follow

Thanks,
Ard/

[PATCH] arm64: efi: correctly align vaddr for runtime maps

[Mark Rutland]

On Thu, Nov 19, 2015 at 07:08:53PM +0100, Ard Biesheuvel wrote:
> On 19 November 2015 at 18:37, Mark Rutland <mark.rutland@arm.com> wrote:
> > The kernel may use a page granularity of 4K, 16K, or 64K depending on
> > configuration.
> >
> > When mapping EFI runtime regions, we use memrange_efi_to_native to round
> > the physical base address of a region down to a granule-aligned
> > boundary, and round the size up to a granule-aligned boundary. However,
> > we fail to similarly round the virtual base address down to a
> > granule-aligned boundary.
> >
> 
> Actually, __create_mapping() (which is called by create_pgd_mapping())
> does the following
> 
> """
> static void  __create_mapping(struct mm_struct *mm, pgd_t *pgd,
>                                     phys_addr_t phys, unsigned long virt,
>                                     phys_addr_t size, pgprot_t prot,
>                                     void *(*alloc)(unsigned long size))
> {
>         unsigned long addr, length, end, next;
> 
>         addr = virt & PAGE_MASK;
>         length = PAGE_ALIGN(size + (virt & ~PAGE_MASK));
> """
> 
> so it does the rounding of the virtual address for us, but we are
> rounding up the length twice.
> I'd rather simply get rid of memrange_efi_to_native() instead, as it
> is obviously redundant.

We'd have to have our own conversion from EFI page size to kernel page
size, but otherwise that would be fine, yes.

I'll spin a v2 to that effect.

> > The virtual base address may be up to PAGE_SIZE - 4K above what it
> > should be, and in create_pgd_mapping, we may erroneously map an
> > additional page at the end of any region which does not have a
> > granule-aligned virtual base address.
> >
> > Depending on the memory map, this page may be in a region we are not
> > intended/permitted to map, or may clash with a different region that we
> > wich to map.
> >
> > Prevent this issue by rounding the virtual base address down to the
> > kernel page granularity, matching what we do for the physical base
> > address.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm64/kernel/efi.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > I spotted this by playing with Will's break-before-make checker [1], which
> > detected an erroneously created PTE being overwritten with a different output
> > address.
> >
> > It looks like the VA bug was introduced in commit f3cdfd239da56a4c ("arm64/efi:
> > move SetVirtualAddressMap() to UEFI stub").
> >
> > Prior to commit 60305db9884515ca ("arm64/efi: move virtmap init to early
> > initcall") so manual fixup is required, but the logic fix is the same.
> >
> 
> I don't follow

Prior to f3cdfd239da56a4c we didn't align the PA and VA separately, so
we didn't have this bug.

In 60305db9884515ca we moved the code around a bit, so the patch won't
apply, but the diff is practically identical.

When running with Will's patch, I spotted the bug due to the additional
page clashing with the mapping created for an adjacent region.

Thanks,
Mark.

[PATCH] arm64: efi: correctly align vaddr for runtime maps

[Ard Biesheuvel]

On 19 November 2015 at 19:17, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Nov 19, 2015 at 07:08:53PM +0100, Ard Biesheuvel wrote:
>> On 19 November 2015 at 18:37, Mark Rutland <mark.rutland@arm.com> wrote:
>> > The kernel may use a page granularity of 4K, 16K, or 64K depending on
>> > configuration.
>> >
>> > When mapping EFI runtime regions, we use memrange_efi_to_native to round
>> > the physical base address of a region down to a granule-aligned
>> > boundary, and round the size up to a granule-aligned boundary. However,
>> > we fail to similarly round the virtual base address down to a
>> > granule-aligned boundary.
>> >
>>
>> Actually, __create_mapping() (which is called by create_pgd_mapping())
>> does the following
>>
>> """
>> static void  __create_mapping(struct mm_struct *mm, pgd_t *pgd,
>>                                     phys_addr_t phys, unsigned long virt,
>>                                     phys_addr_t size, pgprot_t prot,
>>                                     void *(*alloc)(unsigned long size))
>> {
>>         unsigned long addr, length, end, next;
>>
>>         addr = virt & PAGE_MASK;
>>         length = PAGE_ALIGN(size + (virt & ~PAGE_MASK));
>> """
>>
>> so it does the rounding of the virtual address for us, but we are
>> rounding up the length twice.
>> I'd rather simply get rid of memrange_efi_to_native() instead, as it
>> is obviously redundant.
>
> We'd have to have our own conversion from EFI page size to kernel page
> size, but otherwise that would be fine, yes.
>
> I'll spin a v2 to that effect.
>

The size is simply 'md->num_pages << EFI_PAGE_SHIFT'

>> > The virtual base address may be up to PAGE_SIZE - 4K above what it
>> > should be, and in create_pgd_mapping, we may erroneously map an
>> > additional page at the end of any region which does not have a
>> > granule-aligned virtual base address.
>> >
>> > Depending on the memory map, this page may be in a region we are not
>> > intended/permitted to map, or may clash with a different region that we
>> > wich to map.
>> >
>> > Prevent this issue by rounding the virtual base address down to the
>> > kernel page granularity, matching what we do for the physical base
>> > address.
>> >
>> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> > Cc: Will Deacon <will.deacon@arm.com>
>> > ---
>> >  arch/arm64/kernel/efi.c | 6 ++++--
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > I spotted this by playing with Will's break-before-make checker [1], which
>> > detected an erroneously created PTE being overwritten with a different output
>> > address.
>> >
>> > It looks like the VA bug was introduced in commit f3cdfd239da56a4c ("arm64/efi:
>> > move SetVirtualAddressMap() to UEFI stub").
>> >
>> > Prior to commit 60305db9884515ca ("arm64/efi: move virtmap init to early
>> > initcall") so manual fixup is required, but the logic fix is the same.
>> >
>>
>> I don't follow
>
> Prior to f3cdfd239da56a4c we didn't align the PA and VA separately, so
> we didn't have this bug.
>
> In 60305db9884515ca we moved the code around a bit, so the patch won't
> apply, but the diff is practically identical.
>
> When running with Will's patch, I spotted the bug due to the additional
> page clashing with the mapping created for an adjacent region.
>
> Thanks,
> Mark.

[PATCH] arm64: efi: correctly align vaddr for runtime maps

[Mark Rutland]

On Thu, Nov 19, 2015 at 07:29:18PM +0100, Ard Biesheuvel wrote:
> On 19 November 2015 at 19:17, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Nov 19, 2015 at 07:08:53PM +0100, Ard Biesheuvel wrote:
> >> On 19 November 2015 at 18:37, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > The kernel may use a page granularity of 4K, 16K, or 64K depending on
> >> > configuration.
> >> >
> >> > When mapping EFI runtime regions, we use memrange_efi_to_native to round
> >> > the physical base address of a region down to a granule-aligned
> >> > boundary, and round the size up to a granule-aligned boundary. However,
> >> > we fail to similarly round the virtual base address down to a
> >> > granule-aligned boundary.
> >> >
> >>
> >> Actually, __create_mapping() (which is called by create_pgd_mapping())
> >> does the following
> >>
> >> """
> >> static void  __create_mapping(struct mm_struct *mm, pgd_t *pgd,
> >>                                     phys_addr_t phys, unsigned long virt,
> >>                                     phys_addr_t size, pgprot_t prot,
> >>                                     void *(*alloc)(unsigned long size))
> >> {
> >>         unsigned long addr, length, end, next;
> >>
> >>         addr = virt & PAGE_MASK;
> >>         length = PAGE_ALIGN(size + (virt & ~PAGE_MASK));
> >> """
> >>
> >> so it does the rounding of the virtual address for us, but we are
> >> rounding up the length twice.
> >> I'd rather simply get rid of memrange_efi_to_native() instead, as it
> >> is obviously redundant.
> >
> > We'd have to have our own conversion from EFI page size to kernel page
> > size, but otherwise that would be fine, yes.
> >
> > I'll spin a v2 to that effect.
> >
> 
> The size is simply 'md->num_pages << EFI_PAGE_SHIFT'

Indeed, I figured this out immediately after sending the last mail.

It looks much nicer now. I'll post it once I've given it a spin.

Thanks,
Mark.