Re: [PATCH 13/14] KVM: arm64: Restrict hyp stage-1 manipulation in protected mode

[Quentin Perret]

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@xxxxxxxxxx/

> > +                                      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@xxxxxxxxxx/

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