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

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

On 03/21/2014 12:32 PM, Ian Campbell wrote:
>> The toolstack will only provide the gfn, except for this
>> function.
> 
> memory_unmap should also only take a gfn, which Xen should lookup to get
> an mfn. Notice that on x86 the unmap case doesn't use the mfn argument
> and passes only a gfn to clear_mmio_p2m_entry.

The MFN argument is used to check if the domain has access to the
MFN(see iomem_access_permitted at the beginning of the case).

I think we should implement the behavior you described in the common
implementation.

> It's racy to have the toolstack provide it anyway.

>>>> 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.
> 
> I don't think this is a big issue. Having permission to have a mapping
> does not necessarily imply having the actual mapping, if the toolstack
> wants to do it then let it.
> 
>> 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.
> 
> Well, it already can today AFAICS, via remove_from_physmap.

remove_from_physmap can't remove MMIO region. There is a specific check
in get_page_from_gfn for this purpose.

IHMO, memory_unmap should avoid to remove other region than MMIO. Mainly
because apply_p2m_changes might not remove every reference taken on a page.

-- 
Julien Grall