Re: [Xen-devel] [PATCH v6 1/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 1 Aug 2018, Jan Beulich wrote:
> First of all I think your Cc list is too short here - all of REST should be
> included imo.

CC'ing them now. I'll also add them automatically from next time.


> >>> On 31.07.18 at 20:23, <sstabellini@xxxxxxxxxx> wrote:
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -535,6 +535,21 @@ static XSM_INLINE int 
> > xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
> >      return xsm_default_action(action, d, t);
> >  }
> >  
> > +/*
> > + * This action also requires that @current targets @d, but it has already 
> > been
> > + * checked somewhere higher in the call stack.
> 
> I'm not convinced it is a good idea to have such a dependency, even
> more so with this cloudy a reference. If there's another XSM check
> that has necessarily been done before, you should at least name it
> here so it's easy to later verify that the assumption still holds. But
> even better would imo be to re-do the check here, just in case.

I am fine with that. It should be just a matter of doing the following,
right?

static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d,
                                         struct domain *t)
{
    XSM_ASSERT_ACTION(XSM_TARGET);
    return xsm_default_action(XSM_TARGET, current->domain, d) &&
           xsm_default_action(action, current->domain, t);


> > --- a/xen/xsm/flask/hooks.c
> > +++ b/xen/xsm/flask/hooks.c
> > @@ -1198,6 +1198,17 @@ static int flask_map_gmfn_foreign(struct domain *d, 
> > struct domain *t)
> >      return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | 
> > MMU__MAP_WRITE);
> >  }
> >  
> > +/*
> > + * This action also requires that @current has MMU__MAP_READ/WRITE over @d,
> > + * but that has already been checked somewhere higher in the call stack 
> > (for
> > + * example, by flask_add_to_physmap()).
> 
> This one's better in that it at least names the other hook. Still I'm
> not fully convinced that omitting the other half of the check here
> is prudent. We'll see what others think ...

Sure, it should be simple to change (I hope I am not missing something).

static int flask_map_gmfn_share(struct domain *d, struct domain *t)
{
  return current_has_perm(d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) &&
         (current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
          domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM));

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel