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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 2 Aug 2018, Jan Beulich wrote:
> >> > +/*
> >> > + * 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);
> 
> Not exactly - xsm_default_action() doesn't return boolean, nor should
> the function here.

You are right, zero is success. So something like the following:

    int rc = xsm_default_action(XSM_TARGET, current->domain, d);
    if ( rc )
        return rc;
    return 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));
> 
> Same here afaict.

Yep:

    int rc = current_has_perm(d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
    if ( rc )
        return rc;
    rc = current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
    if ( rc )
        return rc;
    return 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