| Message ID | 1447954656-10435-1-git-send-email-mark.rutland@arm.com |
|---|---|
| State | New |
| Headers | show |
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/ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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; }
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 -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel