Re: [XEN PATCH v1 1/1] x86/domctl: add gva_to_gfn command

[Ковалёв Сергей <valor@xxxxxxx>]

20.03.2023 22:07, Andrew Cooper пишет:
> On 20/03/2023 4:32 pm, Ковалёв Сергей wrote:
> > gva_to_gfn command used for fast address translation in LibVMI project.
> > With such a command it is possible to perform address translation in
> > single call instead of series of queries to get every page table.
> >
> > Thanks to Dmitry Isaykin for involvement.
> >
> > Signed-off-by: Sergey Kovalev <valor@xxxxxxx>
> I fully appreciate why you want this hypercall, and I've said several
> times that libvmi wants something better than it has, but...
>
> > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > index 2118fcad5d..0c9706ea0a 100644
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -1364,6 +1364,23 @@ long arch_do_domctl(
> >               copyback = true;
> >           break;
> >
> > +    case XEN_DOMCTL_gva_to_gfn:
> > +    {
> > +        uint64_t ga = domctl->u.gva_to_gfn.addr;
> > +        uint64_t cr3 = domctl->u.gva_to_gfn.cr3;
> > +        struct vcpu* v = d->vcpu[0];
> ... this isn't safe if you happen to issue this hypercall too early in a
> domain's lifecycle.
>
> If nothing else, you want to do a domain_vcpu() check and return -ENOENT
> in the failure case.
Thanks!

> More generally, issuing the hypercall under vcpu0 isn't necessarily
> correct.  It is common for all vCPUs to have equivalent paging settings,
> but e.g. Xen transiently disables CR4.CET and CR0.WP in order to make
> self-modifying code changes.
>
> Furthermore, the setting of CR4.{PAE,PSE} determines reserved bits, so
> you can't even ignore the access rights and hope that the translation
> works out correctly.
Thanks! I didn't think about such things earlier. I should to think
this know carefully.

> Ideally we'd have a pagewalk algorithm which didn't require taking a
> vcpu, and instead just took a set of paging configuration, but it is all
> chronically entangled right now.
Do You mean to add new implementation of "paging_ga_to_gfn_cr3"?

> I think, at a minimum, you need to take a vcpu_id as an input, but I
> suspect to make this a usable API you want an altp2m view id too.
Why we should consider altp2m while translating guest virtual address to
guest physical one?

> Also, I'm pretty sure this is only safe for a paused vCPU.  If the vCPU
> isn't paused, then there's a TOCTOU race in the pagewalk code when
> inspecting control registers.
Thanks! Should we pause the domain?

> > +        uint32_t pfec = PFEC_page_present;
> > +        unsigned int page_order;
> > +
> > +        uint64_t gfn = paging_ga_to_gfn_cr3(v, cr3, ga, &pfec,
> > &page_order);
> > +        domctl->u.gva_to_gfn.addr = gfn;
> > +        domctl->u.gva_to_gfn.page_order = page_order;
> page_order is only not stack rubble if gfn is different to INVALID_GFN.
Sorry but I don't understand "is only not stack rubble". Do you mean
that I should initialize "page_order" while defining it?

> > +        if ( __copy_to_guest(u_domctl, domctl, 1) )
> > +            ret = -EFAULT;
> You want to restrict this to just the gva_to_gfn sub-portion.  No point
> copying back more than necessary.
>
> ~Andrew
Thanks a lot!

--
Best regards,
Sergey Kovalev