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

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Fri, 2014-03-21 at 12:45 +0000, Julien Grall wrote:
> 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.

OK.

> 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.

OK, but then the changes to apply_p2m_changes should be less specific to
this case and usable even for e.g. sanity checking the removal of ram
mappings too.

Ian.


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