From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall Date: Tue, 01 Apr 2014 17:00:43 +0100 Message-ID: <533AE2AB.1080404@linaro.org> References: <1395712976-19454-1-git-send-email-avanzini.arianna@gmail.com> <1395712976-19454-5-git-send-email-avanzini.arianna@gmail.com> <533181EE.1040103@linaro.org> <1396363979.8667.225.camel@kazak.uk.xensource.com> <1396366745.8667.246.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: <1396366745.8667.246.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 Fraser , Stefano Stabellini , Tim Deegan , Dario Faggioli , "xen.org" , "xen-devel@lists.xen.org" , Julien Grall , Eric Trudeau , Jan Beulich , Arianna Avanzini , "viktor.kleinik@globallogic.com" List-Id: xen-devel@lists.xenproject.org On 04/01/2014 04:39 PM, Ian Campbell wrote: > On Tue, 2014-04-01 at 16:16 +0100, Julien Grall wrote: >> On 04/01/2014 03:52 PM, Ian Campbell wrote: >>> On Tue, 2014-03-25 at 13:17 +0000, Julien Grall wrote: >>>>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c >>>>> index 7cf610a..ae120d9 100644 >>>>> --- a/xen/common/domctl.c >>>>> +++ b/xen/common/domctl.c >>>>> @@ -818,6 +818,71 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) >>>>> } >>>>> break; >>>>> >>>>> + case XEN_DOMCTL_memory_mapping: >>>>> + { >>>>> + unsigned long gfn = op->u.memory_mapping.first_gfn; >>>>> + unsigned long mfn = op->u.memory_mapping.first_mfn; >>>>> + unsigned long nr_mfns = op->u.memory_mapping.nr_mfns; >>>>> + unsigned long mfn_end = mfn + nr_mfns; >>>>> + unsigned long gfn_end = gfn + nr_mfns; >>>>> + int add = op->u.memory_mapping.add_mapping; >>>>> + >>>>> + ret = -EINVAL; >>>>> + if ( (mfn_end - 1) < mfn || /* wrap? */ >>>>> + ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) || >>>>> + (gfn_end - 1) < gfn ) /* wrap? */ >>>>> + return ret; >>>> >>>> >>>> Would not it be better to only rely on the GFN when the toolstack is >>>> removing the mapping? >>> >>> I'm not sure I get what you mean, the gfn is an input to add as well. >>> >>>> I know this is not the previous behavior, but most of the hypercall >>>> which deal with removing mapping only take GFN. >>> >>> Regardless of my confusion above any semantic change should be done >>> separately from the move of the code from x86 to generic and whatever >>> minor updates that entails for the ARM case. >> >> >> I didn't intend to ask "remove the field mfn". When a mapping is >> removed, the MFN is useless because we can retrieve it from the GFN. >> >> It won't impact x86, because removing a GFN that doesn't have mapping >> should also fail. >> >> When I was reading the code, x86 seems to lack of some check during >> the removing part: a privileged guest (e.g a domain that have permission >> to remove a MMIO range) can remove any MMIO range as long as the >> MFN is in the permitted range. So the MFN may not be mapped to the GFN. >> >> This will result to a mismatch between the actually mapped MFN >> and the permitted range. > > Is the solution to that not to add the checks rather than remove them? Which check? To give an example: In the p2m we have this list of directly MMIO gfn 0xab => mfn 0x2b gfn 0xac => mfn 0x2c gfn 0xad => mfn 0x3b gfn 0xae => mfn 0x3c The current domain as permission on: 0x2b-0x2c It's valid (but wrong) to call DOMCTL_memory_mapping with: add = 0 gfn = 0xac mfn = 0x2b nr_mfns = 1 As you can see mfn doesn't match the gfn, but the code will let you do that. What I suggest is two have something similar to: for (i = 0; i < nr_mfns; i++) { mfn = p2m_lookup(d, gfn); clear p2m remove rigth } -- Julien Grall