Hi Jan, Thanks for reviewing my patch.
2017-08-23 17:55 GMT+08:00 Jan Beulich <jbeul...@suse.com>: >>>> On 22.08.17 at 20:08, <blacksk...@gmail.com> 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 ... This might be confusing. But think of returning 0 as "allowed", the only condition where this statement returns a 0 is when both calls return 0 -- so it's actually an "and". (Think of de-morgan's law.) But as Stefano has pointed out, I should preserve the error code. And as Daniel has pointed out, I should also check if d and t can share memory. > >> --- 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. I agree with what Stefano has said below. Cheers, Zhongze Liu. > > Jan > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel