Re: [Xen-devel] [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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?

Ian.


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