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:08 +0000, Julien Grall wrote:
> 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.

And REMOVE doesn't check it so this isn't actually achieving anything
today.

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

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.

Ian.


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