Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.08.17 at 19:16, <sstabellini@xxxxxxxxxx> wrote:
> On Wed, 23 Aug 2017, Jan Beulich wrote:
>> >>> On 22.08.17 at 20:08, <blackskygg@xxxxxxxxx> wrote:
>> > The original xsm_map_gmfn_foregin policy checks if source domain has the 
>> > proper
>> > privileges over the target domain. Under this policy, it's not allowed if 
>> > a Dom0
>> > wants to map pages from one DomU to another, this restricts some useful 
>> > yet not
>> > dangerous usages of the API, such as sharing pages among DomU's by calling
>> > XENMEM_add_to_physmap from Dom0.
>> > 
>> > Change the policy to: IIF current domain has the proper privilege on the
>> > target domain and source domain, grant the access.
>> 
>> You say "and here", yet ...
>> 
>> > --- a/xen/include/xsm/dummy.h
>> > +++ b/xen/include/xsm/dummy.h
>> > @@ -525,10 +525,12 @@ static XSM_INLINE int 
>> > xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>> >      return xsm_default_action(action, d1, d2);
>> >  }
>> >  
>> > -static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 
>> > *d, struct domain *t)
>> > +static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 
>> > *cd,
>> > +                                           struct domain *d, struct 
>> > domain *t)
>> >  {
>> >      XSM_ASSERT_ACTION(XSM_TARGET);
>> > -    return xsm_default_action(action, d, t);
>> > +    return xsm_default_action(action, cd, d) ||
>> > +        xsm_default_action(action, cd, t);
>> >  }
>> 
>> ... you use "or" here and ...
>> 
>> > --- a/xen/xsm/flask/hooks.c
>> > +++ b/xen/xsm/flask/hooks.c
>> > @@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain 
>> > *d1, struct domain *d2)
>> >      return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
>> >  }
>> >  
>> > -static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
>> > +static int flask_map_gmfn_foreign(struct domain *cd,
>> > +                                  struct domain *d, struct domain *t)
>> >  {
>> > -    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | 
>> > MMU__MAP_WRITE);
>> > +    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | 
>> > MMU__MAP_WRITE) ||
>> > +        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | 
>> > MMU__MAP_WRITE);
>> >  }
>> 
>> ... here. A domain can't have XSM_TARGET permission over two
>> other domains, so what you want to do here can't work at all,
>> afaict.
> 
> It would work with XSM_TARGET if cd == d, and cd has XSM_TARGET
> permission over t (current case). Otherwise, it would work if cd is
> XSM_PRIV (Zhongze's case). Did I get it wrong?

I think so, yes, but besides the "and" vs "or" discrepancy the patch
description suggests a three-way operation (i.e. including the case
of all three domains being different ones). The case you describe
doesn't require three domains to be passed into the hook, it would
- afaict - just be the traditional "cd is XSM_TARGET over t" case
(matching for example xsm_evtchn_{status,unbound}()).

Jan


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