Re: [Xen-devel] [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Tue, 2014-04-01 at 17:00 +0100, Julien Grall wrote:
> >>>>> +        ret = -EINVAL;
> >>>>> +        if ( (mfn_end - 1) < mfn || /* wrap? */
> >>>>> +             ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> >>>>> +             (gfn_end - 1) < gfn ) /* wrap? */
> >>>>> +            return ret;
> > 
> > Is the solution to that not to add the checks rather than remove them?
> 
> Which check?

You were, I thought, proposing to remove some of the check quoted above
in the remove case? I was asking why we wouldn't instead check that the
given mfn is mapped by the given gfn.

>  To give an example:
>    In the p2m we have this list of directly MMIO
>          gfn 0xab => mfn 0x2b
>          gfn 0xac => mfn 0x2c
>          gfn 0xad => mfn 0x3b
>        gfn 0xae => mfn 0x3c
>    The current domain as permission on: 0x2b-0x2c
> 
> It's valid (but wrong) to call DOMCTL_memory_mapping with:
>    add = 0
>    gfn = 0xac
>    mfn = 0x2b
>    nr_mfns = 1
> 
> As you can see mfn doesn't match the gfn, but the code will let you do that.
> 
> What I suggest is two have something similar to:
>     for (i = 0; i < nr_mfns; i++)
>     {
>        mfn = p2m_lookup(d, gfn);
>        clear p2m
>        remove rigth
>     }

Over the range of gfns given by the hypercall this algorithm is O(n^2)
which we don't want. And it iterated over all guest mfns which is also
something we don't want, or at least we don't want to add new instances
of.

Wouldn't it be better to do the p2m_lookup and check that the result
equates to the mfn given in the argument?

This interface is already pretty broken for the case where a toolstack
maps the same mfns into a guest twice. But that is a pretty broken thing
for a toolstack to do. We could perhaps just modify this hypercall to
fail if the guest already has permission to access the given mfn, the
assumption being that that permissions must be given by
DOMCTL_memory_mapping and not DOMCTL_iomem_perm.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel