On 16/02/18 09:25, Christoffer Dall wrote:
> On Thu, Feb 15, 2018 at 01:52:05PM +0000, Marc Zyngier wrote:
>> On 18/01/18 14:39, Christoffer Dall wrote:
>>> On Thu, Jan 04, 2018 at 06:43:29PM +0000, Marc Zyngier wrote:
>>>> We so far mapped our HYP IO (which is essencially the GICv2 control
>>>> registers) using the same method as for memory. It recently appeared
>>>> that is a bit unsafe:
>>>>
>>>> We compute the HYP VA using the kern_hyp_va helper, but that helper
>>>> is only designed to deal with kernel VAs coming from the linear map,
>>>> and not from the vmalloc region... This could in turn cause some bad
>>>> aliasing between the two, amplified by the upcoming VA randomisation.
>>>>
>>>> A solution is to come up with our very own basic VA allocator for
>>>> MMIO. Since half of the HYP address space only contains a single
>>>> page (the idmap), we have plenty to borrow from. Let's use the idmap
>>>> as a base, and allocate downwards from it. GICv2 now lives on the
>>>> other side of the great VA barrier.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>>>> ---
>>>> virt/kvm/arm/mmu.c | 56 +++++++++++++++++++++++++++++++++++++++++-------------
>>>> 1 file changed, 43 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>>>> index 6192d45d1e1a..14c5e5534f2f 100644
>>>> --- a/virt/kvm/arm/mmu.c
>>>> +++ b/virt/kvm/arm/mmu.c
>>>> @@ -43,6 +43,9 @@ static unsigned long hyp_idmap_start;
>>>> static unsigned long hyp_idmap_end;
>>>> static phys_addr_t hyp_idmap_vector;
>>>>
>>>> +static DEFINE_MUTEX(io_map_lock);
>>>> +static unsigned long io_map_base;
>>>> +
>>>> #define S2_PGD_SIZE (PTRS_PER_S2_PGD * sizeof(pgd_t))
>>>> #define hyp_pgd_order get_order(PTRS_PER_PGD * sizeof(pgd_t))
>>>>
>>>> @@ -502,27 +505,31 @@ static void unmap_hyp_range(pgd_t *pgdp, phys_addr_t start, u64 size)
>>>> *
>>>> * Assumes hyp_pgd is a page table used strictly in Hyp-mode and
>>>> * therefore contains either mappings in the kernel memory area (above
>>>> - * PAGE_OFFSET), or device mappings in the vmalloc range (from
>>>> - * VMALLOC_START to VMALLOC_END).
>>>> + * PAGE_OFFSET), or device mappings in the idmap range.
>>>> *
>>>> - * boot_hyp_pgd should only map two pages for the init code.
>>>> + * boot_hyp_pgd should only map the idmap range, and is only used in
>>>> + * the extended idmap case.
>>>> */
>>>> void free_hyp_pgds(void)
>>>> {
>>>> + pgd_t *id_pgd;
>>>> +
>>>> mutex_lock(&kvm_hyp_pgd_mutex);
>>>>
>>>> + id_pgd = boot_hyp_pgd ? boot_hyp_pgd : hyp_pgd;
>>>> +
>>>> + if (id_pgd)
>>>> + unmap_hyp_range(id_pgd, io_map_base,
>>>> + hyp_idmap_start + PAGE_SIZE - io_map_base);
>>>> +
>>>> if (boot_hyp_pgd) {
>>>> - unmap_hyp_range(boot_hyp_pgd, hyp_idmap_start, PAGE_SIZE);
>>>> free_pages((unsigned long)boot_hyp_pgd, hyp_pgd_order);
>>>> boot_hyp_pgd = NULL;
>>>> }
>>>>
>>>> if (hyp_pgd) {
>>>> - unmap_hyp_range(hyp_pgd, hyp_idmap_start, PAGE_SIZE);
>>>> unmap_hyp_range(hyp_pgd, kern_hyp_va(PAGE_OFFSET),
>>>> (uintptr_t)high_memory - PAGE_OFFSET);
>>>> - unmap_hyp_range(hyp_pgd, kern_hyp_va(VMALLOC_START),
>>>> - VMALLOC_END - VMALLOC_START);
>>>>
>>>> free_pages((unsigned long)hyp_pgd, hyp_pgd_order);
>>>> hyp_pgd = NULL;
>>>> @@ -721,7 +728,8 @@ int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
>>>> void __iomem **kaddr,
>>>> void __iomem **haddr)
>>>> {
>>>> - unsigned long start, end;
>>>> + pgd_t *pgd = hyp_pgd;
>>>> + unsigned long base;
>>>> int ret;
>>>>
>>>> *kaddr = ioremap(phys_addr, size);
>>>> @@ -733,17 +741,38 @@ int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
>>>> return 0;
>>>> }
>>>>
>>>> + mutex_lock(&io_map_lock);
>>>> +
>>>> + base = io_map_base - size;
>>>
>>> are we guaranteed that hyp_idmap_start (and therefore io_map_base) is
>>> sufficiently greater than 0 ? I suppose that even if RAM starts at 0,
>>> and the kernel was loaded at 0, the idmap page for Hyp would be at some
>>> reasonable offset from the start of the kernel image?
>>
>> On my kernel image:
>> ffff000008080000 t _head
>> ffff000008cc6000 T __hyp_idmap_text_start
>> ffff000009aaa000 B swapper_pg_end
>>
>> _hyp_idmap_text_start is about 12MB from the beginning of the image, and
>> about 14MB from the end. Yes, it is a big kernel. But we're only mapping
>> a few pages there, even with my upcoming crazy vector remapping crap. So
>> the likeliness of this failing is close to zero.
>>
>> Now, close to zero is not necessarily close enough. What I could do is
>> to switch the allocator around on failure, so that if we can't allocate
>> on one side, we can at least try to allocate on the other side. I'm
>> pretty sure we'll never trigger that code, but I can implement it if you
>> think that's worth it.
>>
>
> I don't think we should necessarily implement it, my main concern is
> when reading the code, the reader has to concince himself/herself why
> this is always safe (and not just very likely to be safe). I'm fine
> with adding a comment that explains this instead of implementing
> complicated logic though. What do you think?
Oh, absolutely. I'll add a blurb about this.
>
>>>
>>>> + base &= ~(size - 1);
>>>
>>> I'm not sure I understand this line. Wouldn't it make more sense to use
>>> PAGE_SIZE here?
>>
>> This is trying to align the base of the allocation to its natural size
>> (8kB aligned on an 8kB boundary, for example), which is what other
>> allocators in the kernel do. I've now added a roundup_pow_of_two(size)
>> so that we're guaranteed to deal with those.
>>
>
> Ah right, it's just that I wasn't thinking of the size in terms of
> always being page aligned, so couldn't you here attempt two 4K
> allocations on a 64K page system, where one would overwrite the other?
Ah, I see what you mean. Well spotted. I'll turn that into a
max(roundup_pow_of_two(size), PAGE_SIZE).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
