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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 04/01/2014 03:52 PM, Ian Campbell wrote:
> On Tue, 2014-03-25 at 13:17 +0000, Julien Grall wrote:
>>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>>> index 7cf610a..ae120d9 100644
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -818,6 +818,71 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>>> u_domctl)
>>>      }
>>>      break;
>>>
>>> +    case XEN_DOMCTL_memory_mapping:
>>> +    {
>>> +        unsigned long gfn = op->u.memory_mapping.first_gfn;
>>> +        unsigned long mfn = op->u.memory_mapping.first_mfn;
>>> +        unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
>>> +        unsigned long mfn_end = mfn + nr_mfns;
>>> +        unsigned long gfn_end = gfn + nr_mfns;
>>> +        int add = op->u.memory_mapping.add_mapping;
>>> +
>>> +        ret = -EINVAL;
>>> +        if ( (mfn_end - 1) < mfn || /* wrap? */
>>> +             ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
>>> +             (gfn_end - 1) < gfn ) /* wrap? */
>>> +            return ret;
>>
>>
>> Would not it be better to only rely on the GFN when the toolstack is
>> removing the mapping?
>
> I'm not sure I get what you mean, the gfn is an input to add as well.
>
>> I know this is not the previous behavior, but most of the hypercall
>> which deal with removing mapping only take GFN.
>
> Regardless of my confusion above any semantic change should be done
> separately from the move of the code from x86 to generic and whatever
> minor updates that entails for the ARM case.


I didn't intend to ask "remove the field mfn". When a mapping is
removed, the MFN is useless because we can retrieve it from the GFN.

It won't impact x86, because removing a GFN that doesn't have mapping
should also fail.

When I was reading the code, x86 seems to lack of some check during
the removing part:  a privileged guest (e.g a domain that have permission
to remove a MMIO range) can remove any MMIO range as long as the
MFN is in the permitted range. So the MFN may not be mapped to the GFN.

This will result to a mismatch between the actually mapped MFN
and the permitted range.

Regards,

-- 
Julien Grall

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