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

[Julien Grall <julien.grall@linaro.org>]

On 04/01/2014 04:39 PM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 16:16 +0100, Julien Grall wrote:
>> 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.
> 
> Is the solution to that not to add the checks rather than remove them?

Which check? 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
    }

-- 
Julien Grall