[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |