arm64: efi: correctly align vaddr for runtime maps

PATCH New Mark Rutland 2 participants 5 messages Nov 19, 2015

Subsystems (2)

arm64 EFI

lore

Share & tools

lore

Mark Rutland

author reviewer

2015-11-19 17:37 UTC #0 lore raw

PATCH 1/1 arm64: efi: correctly align vaddr for runtime maps

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 <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Leif Lindholm <[email protected]>

Cc Will Deacon <[email protected]>

---
 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

arch/arm64/kernel/efi.c +4 -2

index de46b50..7855b69 100644
@@ -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;
 }

Signature · 1 line

1.9.1

Ard Biesheuvel

2015-11-19 18:08 UTC #1 lore raw

PATCH 1/1 arm64: efi: correctly align vaddr for runtime maps

Quoted 9 lines · 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.

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.

Quoted 26 lines

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 <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Leif Lindholm <[email protected]>
Cc Will Deacon <[email protected]>
---
 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.

Quoted 6 lines

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/

Mark Rutland

author reviewer

2015-11-19 18:17 UTC #2 lore raw

PATCH 1/1 arm64: efi: correctly align vaddr for runtime maps

Quoted 24 lines · Ard Biesheuvel

Quoted 9 lines · 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.
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));

Quoted 6 lines

"""
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.

Quoted 26 lines

Quoted 26 lines
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 <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Leif Lindholm <[email protected]>
Cc Will Deacon <[email protected]>
---
 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.

Quoted 8 lines

Quoted 6 lines
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.

Ard Biesheuvel

2015-11-19 18:29 UTC #3 lore raw

PATCH 1/1 arm64: efi: correctly align vaddr for runtime maps

Quoted 32 lines · Mark Rutland

Quoted 24 lines · Ard Biesheuvel
Quoted 9 lines · 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.
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));
Quoted 6 lines
"""
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.

Quoted 5 lines

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'

Quoted 46 lines

Quoted 26 lines
Quoted 26 lines
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 <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Leif Lindholm <[email protected]>
Cc Will Deacon <[email protected]>
---
 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.
Quoted 8 lines
Quoted 6 lines
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.

Mark Rutland

author reviewer

2015-11-19 18:32 UTC #4 lore raw

PATCH 1/1 arm64: efi: correctly align vaddr for runtime maps

Quoted 33 lines · Ard Biesheuvel

Quoted 32 lines · Mark Rutland
Quoted 30 lines · Ard Biesheuvel
Quoted 9 lines · 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.
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.

Quoted 7 lines

Quoted 5 lines
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.