From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes Date: Fri, 21 Mar 2014 12:08:49 +0000 Message-ID: <532C2BD1.7040800@linaro.org> References: <1394914286-29713-1-git-send-email-avanzini.arianna@gmail.com> <1394914286-29713-3-git-send-email-avanzini.arianna@gmail.com> <1395398657.27358.27.camel@kazak.uk.xensource.com> <532C27B3.4050802@citrix.com> <1395402840.27358.66.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1395402840.27358.66.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: paolo.valente@unimore.it, keir@xen.org, stefano.stabellini@eu.citrix.com, Ian.Jackson@eu.citrix.com, dario.faggioli@citrix.com, tim@xen.org, xen-devel@lists.xen.org, etrudeau@broadcom.com, JBeulich@suse.com, Arianna Avanzini , viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.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