Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

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

On 03/21/2014 11:54 AM, Ian Campbell wrote:
> On Fri, 2014-03-21 at 11:51 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 03/21/2014 10:44 AM, Ian Campbell wrote:
>>> On Sat, 2014-03-15 at 21:11 +0100, Arianna Avanzini wrote:
>>>> Currently, the REMOVE case of the switch in apply_p2m_changes()
>>>> does not perform any consistency check on the mapping to be removed.
>>>> More in detail, the code does not check that the type of the entry
>>>> is correct in case of I/O memory mapping removal; also, the code
>>>> does not check if the guest address to be unmapped is actually mapped
>>>> to the machine address given as a parameter.
>>>> This commit attempts to add the above-described consistency checks
>>>> to the REMOVE path of apply_p2m_changes(). This is instrumental to
>>>> the following commit which implements the possibility to trigger
>>>> the removal of p2m ranges via the memory_mapping DOMCTL for ARM.
>>>
>>> I'm not sure I follow why this is needed, is there some reason
>>> apply_p2m_changes(REMOVE, ...) should not just remove whatever it is
>>> asked to? What is the downside if the memory_mapping domctl removes
>>> something which is not a memory mapping?
>>>
>>> If it's just "a bug" then I think the toolstack should "Not Do That
>>> Then". If the bug might have security implications then perhaps we need
>>> to worry about it, but do you have such a case in mind?
>>
>> We have to check somewhere that the removed gfn corresponding to the mfn.
> 
> Why? The toolstack can punch whatever holes it wants in the guest
> address space, can't it?

No, every call to apply_p2m_changes is used with a valid mfn given by
Xen directly. The toolstack will only provide the gfn, except for this
function.

> 
>> Otherwise the toolstack may be able to remove any page as long as the
>> MFN is in the iomem permitted range.
> 
> Can't it already do this through other paths?
> 
> Maybe there is a security implication there, but I would hope that the
> two permissions were pretty closely linked.

One the main problem is iomem range permitted won't be anymore in sync.

x86 at least check that the gfn is an MMIO. I think we can safely extend
to check that the GFN use the corresponding MFN.

I don't agree to let the toolstack uses this DOMCTL to do remove any
page in the guess memory.

Regards,

-- 
Julien Grall