lkml.org 
[lkml]   [2021]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 13/14] KVM: arm64: Restrict hyp stage-1 manipulation in protected mode
    Hi Fuad,

    On Wednesday 21 Jul 2021 at 11:45:53 (+0100), Fuad Tabba wrote:
    > > +static int hyp_range_is_shared_walker(u64 addr, u64 end, u32 level,
    > > + kvm_pte_t *ptep,
    > > + enum kvm_pgtable_walk_flags flag,
    > > + void * const arg)
    > > +{
    > > + enum kvm_pgtable_prot prot;
    > > + kvm_pte_t pte = *ptep;
    > > +
    > > + if (!kvm_pte_valid(pte))
    > > + return -EPERM;
    > > +
    > > + prot = kvm_pgtable_hyp_pte_prot(pte);
    > > + if (!prot)
    > > + return -EPERM;
    > nit: is this check necessary?

    I guess not, the next one should do, I'll remove :)

    > > + /* Check that the page has been shared with the hypervisor before */
    > > + if (prot != (PAGE_HYP | KVM_PGTABLE_STATE_SHARED | KVM_PGTABLE_STATE_BORROWED))
    > > + return -EPERM;
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static int hyp_range_is_shared(phys_addr_t start, phys_addr_t end)
    > > +{
    > > + struct kvm_pgtable_walker walker = {
    > > + .cb = hyp_range_is_shared_walker,
    > > + .flags = KVM_PGTABLE_WALK_LEAF,
    > > + };
    > > +
    > > + return kvm_pgtable_walk(&pkvm_pgtable, (u64)__hyp_va(start),
    > > + end - start, &walker);
    > > +}
    > > +
    > > +static int check_host_share_hyp_walker(u64 addr, u64 end, u32 level,
    >
    > nit: It seems the convention is usually addr,size or start,end. Here
    > you're using addr,end.

    Well for some reason all the walkers in pgtable.c follow the addr,end
    convention, so I followed that. But in fact, as per [1] I might actually
    get rid of this walker in v2, so hopefully that'll make the issue go
    away.

    [1] https://lore.kernel.org/kvmarm/YPbwmVk1YD9+y7tr@google.com/

    > > + kvm_pte_t *ptep,
    > > + enum kvm_pgtable_walk_flags flag,
    > > + void * const arg)
    > > +{
    > > + enum kvm_pgtable_prot prot;
    > > + kvm_pte_t pte = *ptep;
    > > +
    > > + /* If invalid, only allow to share pristine pages */
    > > + if (!kvm_pte_valid(pte))
    > > + return pte ? -EPERM : 0;
    > > +
    > > + prot = kvm_pgtable_stage2_pte_prot(pte);
    > > + if (!prot)
    > > + return -EPERM;
    > > +
    > > + /* Cannot share a page that is not owned */
    > > + if (prot & KVM_PGTABLE_STATE_BORROWED)
    > > + return -EPERM;
    > > +
    > > + /* Cannot share a page with restricted access */
    > > + if ((prot & KVM_PGTABLE_PROT_RWX) ^ KVM_PGTABLE_PROT_RWX)
    > nit: isn't this clearer as
    >
    > if ((prot & KVM_PGTABLE_PROT_RWX) != KVM_PGTABLE_PROT_RWX)

    I guess it would be, I'll fix it up.

    > > + return -EPERM;
    > > +
    > > + /* Allow double-sharing (requires cross-checking the hyp stage-1) */
    > > + if (prot & KVM_PGTABLE_STATE_SHARED)
    > > + return hyp_range_is_shared(addr, addr + 1);
    >
    > Why addr+1, rather than end?

    Because 'end' here is the 'end' that was passed to kvm_pgtable_walk()
    IIRC. What I want to do here is check if the page I'm currently visiting
    is already shared and if so, that it is shared with the hypervisor. But
    it's possible that only one page in the range of pages passed to
    __pkvm_host_share_hyp is already shared, so I need to check them one by
    one.

    Anyways, as per the discussion with Marc on [2], I'll probably switch to
    only accept sharing one page at a time, so all these issues should just
    go away as well!

    [2] https://lore.kernel.org/kvmarm/YPa6BGuUFjw8do+o@google.com/

    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static int check_host_share_hyp(phys_addr_t start, phys_addr_t end)
    > > +{
    > > + struct kvm_pgtable_walker walker = {
    > > + .cb = check_host_share_hyp_walker,
    > > + .flags = KVM_PGTABLE_WALK_LEAF,
    > > + };
    > > +
    > > + return kvm_pgtable_walk(&host_kvm.pgt, start, end - start, &walker);
    > > +}
    > > +
    > > +int __pkvm_host_share_hyp(phys_addr_t start, phys_addr_t end)
    > > +{
    > > + enum kvm_pgtable_prot prot;
    > > + int ret;
    > > +
    > > + if (!range_is_memory(start, end))
    > > + return -EINVAL;
    > > +
    > > + hyp_spin_lock(&host_kvm.lock);
    > > + hyp_spin_lock(&pkvm_pgd_lock);
    > > +
    > > + ret = check_host_share_hyp(start, end);
    > > + if (ret)
    > > + goto unlock;
    > > +
    > > + prot = KVM_PGTABLE_PROT_RWX | KVM_PGTABLE_STATE_SHARED;
    > > + ret = host_stage2_idmap_locked(start, end, prot);
    >
    > Just for me to understand this better. The id mapping here, which
    > wasn't taking place before this patch, is for tracking, right?

    Yes, exactly, I want to make sure to mark the page as shared (and
    borrowed) in the relevant page-tables to not forget about it :)

    Cheers,
    Quentin

    \
     
     \ /
      Last update: 2021-07-21 15:38    [W:8.218 / U:0.228 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site