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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.08.18 at 00:21, <sstabellini@xxxxxxxxxx> wrote:
> 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);

Not exactly - xsm_default_action() doesn't return boolean, nor should
the function here.

>> > --- 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.

Jan



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