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

[Ard Biesheuvel]

On 19 November 2015 at 19:17, Mark Rutland <mark.rutland at 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 at 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 at arm.com>
>> > Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> > Cc: Catalin Marinas <catalin.marinas at arm.com>
>> > Cc: Leif Lindholm <leif.lindholm at linaro.org>
>> > Cc: Will Deacon <will.deacon at 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.