Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
On 08/24/2017 08:39 AM, Jan Beulich wrote: On 24.08.17 at 13:33,wrote: Hi Jan, 2017-08-24 14:37 GMT+08:00 Jan Beulich : On 24.08.17 at 02:51, wrote: 2017-08-23 17:55 GMT+08:00 Jan Beulich : On 22.08.17 at 20:08, wrote: --- 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. Ah, right - the code as written suggests boolean return values, which gives it the wrong look. You really mean ?: instead of ||. Why do you, btw, pass in current->domain (as cd) instead of obtaining it here (just like various other hooks do)? This was my original plan, but I'm not very sure about this, so I turn to Julien for help, and... Here is part of the irc log from a discussion with Julien on #xendevel, where Julien said: blackskygg: I think you want to pass the current domain in parameter, i.e having 3 domains argument. because your solution only works when XSM is not enabled (this is the dummy callback) when XSM is enabled, the policy would be specificed by the administrator he needs to be able to know which domain was doing the configuration. in flask/hooks.c there are quite a few uses of avc_current_has_perm() in such cases, so I would think not handing current->domain through the hook should be fine. But of course Daniel may tell you I'm completely wrong here. Jan This is really just a choice of whatever looks better. There's a very minor performance penalty from not calling current->domain over and over, but there might also be a performance gain if current_has_perm is not inlined and the call results in smaller code size. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
>>> On 24.08.17 at 13:33,wrote: > Hi Jan, > > 2017-08-24 14:37 GMT+08:00 Jan Beulich : > On 24.08.17 at 02:51, wrote: >>> 2017-08-23 17:55 GMT+08:00 Jan Beulich : >>> On 22.08.17 at 20:08, wrote: > --- 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. >> >> Ah, right - the code as written suggests boolean return values, >> which gives it the wrong look. You really mean ?: instead of ||. >> Why do you, btw, pass in current->domain (as cd) instead of >> obtaining it here (just like various other hooks do)? > > This was my original plan, but I'm not very sure about this, so I turn > to Julien for help, and... > Here is part of the irc log from a discussion with Julien on > #xendevel, where Julien said: > >blackskygg: I think you want to pass the current domain in > parameter, i.e having 3 domains argument. >because your solution only works when XSM is not enabled (this is > the dummy callback) >when XSM is enabled, the policy would be specificed by the administrator >he needs to be able to know which domain was doing the configuration. in flask/hooks.c there are quite a few uses of avc_current_has_perm() in such cases, so I would think not handing current->domain through the hook should be fine. But of course Daniel may tell you I'm completely wrong here. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
Hi Jan, 2017-08-24 14:37 GMT+08:00 Jan Beulich: On 24.08.17 at 02:51, wrote: >> 2017-08-23 17:55 GMT+08:00 Jan Beulich : >> On 22.08.17 at 20:08, wrote: --- 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. > > Ah, right - the code as written suggests boolean return values, > which gives it the wrong look. You really mean ?: instead of ||. > Why do you, btw, pass in current->domain (as cd) instead of > obtaining it here (just like various other hooks do)? This was my original plan, but I'm not very sure about this, so I turn to Julien for help, and... Here is part of the irc log from a discussion with Julien on #xendevel, where Julien said: blackskygg: I think you want to pass the current domain in parameter, i.e having 3 domains argument. because your solution only works when XSM is not enabled (this is the dummy callback) when XSM is enabled, the policy would be specificed by the administrator he needs to be able to know which domain was doing the configuration. Cheers, Zhongze Liu > > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
>>> On 24.08.17 at 02:51,wrote: > 2017-08-23 17:55 GMT+08:00 Jan Beulich : > On 22.08.17 at 20:08, wrote: >>> --- 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. Ah, right - the code as written suggests boolean return values, which gives it the wrong look. You really mean ?: instead of ||. Why do you, btw, pass in current->domain (as cd) instead of obtaining it here (just like various other hooks do)? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
>>> On 23.08.17 at 19:16,wrote: > On Wed, 23 Aug 2017, Jan Beulich wrote: >> >>> On 22.08.17 at 20:08, 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@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
Hi Jan, Thanks for reviewing my patch. 2017-08-23 17:55 GMT+08:00 Jan Beulich: On 22.08.17 at 20:08, 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
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
On Wed, 23 Aug 2017, Jan Beulich wrote: > >>> On 22.08.17 at 20:08,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? ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
On 08/22/2017 10:18 PM, Zhongze Liu wrote: Hi Stefano, 2017-08-23 3:58 GMT+08:00 Stefano Stabellini: On Wed, 23 Aug 2017, Zhongze Liu 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 ^ IFF target domain and source domain, grant the access. References to this xsm check have also been updated to pass the current domain as a new parameter. This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]). [1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html Signed-off-by: Zhongze Liu Cc: Stefano Stabellini Cc: Julien Grall Cc: George Dunlap Cc: Jan Beulich Cc: Andrew Cooper Cc: Daniel De Graaf Cc: xen-devel@lists.xen.org --- xen/arch/arm/mm.c | 2 +- xen/arch/x86/mm/p2m.c | 2 +- xen/include/xsm/dummy.h | 6 -- xen/include/xsm/xsm.h | 7 --- xen/xsm/flask/hooks.c | 6 -- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index a810a056d7..9ec78d8c03 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one( return -EINVAL; } -rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od); +rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od); if ( rc ) { rcu_unlock_domain(od); diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index e8a57d118c..a547fd00c0 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, if ( tdom == fdom ) goto out; -rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom); +rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom); if ( rc ) goto out; diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 62fcea6f04..28dbc6f2a2 100644 --- 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); We need to preserve the returned errors: rc = xsm_default_action(action, cd, d); if (rc) return rc; return xsm_default_action(action, cd, t); OK, will correct this. } static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op) diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 60c0fd6a62..a20654a803 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -85,7 +85,7 @@ struct xsm_operations { int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct page_info *page); int (*add_to_physmap) (struct domain *d1, struct domain *d2); int (*remove_from_physmap) (struct domain *d1, struct domain *d2); -int (*map_gmfn_foreign) (struct domain *d, struct domain *t); +int (*map_gmfn_foreign) (struct domain *cd, struct domain *d, struct domain *t); int (*claim_pages) (struct domain *d); int (*console_io) (struct domain *d, int cmd); @@ -372,9 +372,10 @@ static inline int xsm_remove_from_physmap(xsm_default_t def, struct domain *d1, return xsm_ops->remove_from_physmap(d1, d2); } -static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *d, struct domain *t) +static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *cd, +struct domain *d, struct domain *t) { -return xsm_ops->map_gmfn_foreign(d, t); +return xsm_ops->map_gmfn_foreign(cd, d, t); } static inline int xsm_claim_pages(xsm_default_t def, struct domain *d) diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 91146275bb..3408b6b9e1 100644 --- 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
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
>>> On 22.08.17 at 20:08,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. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
Hi Stefano, 2017-08-23 3:58 GMT+08:00 Stefano Stabellini: > On Wed, 23 Aug 2017, Zhongze Liu 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 > ^ IFF > > >> target domain and source domain, grant the access. >> >> References to this xsm check have also been updated to pass the current >> domain as a new parameter. >> >> This is for the proposal "Allow setting up shared memory areas between VMs >> from xl config file" (see [1]). >> >> [1] >> https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html >> >> Signed-off-by: Zhongze Liu >> >> Cc: Stefano Stabellini >> Cc: Julien Grall >> Cc: George Dunlap >> Cc: Jan Beulich >> Cc: Andrew Cooper >> Cc: Daniel De Graaf >> Cc: xen-devel@lists.xen.org >> --- >> xen/arch/arm/mm.c | 2 +- >> xen/arch/x86/mm/p2m.c | 2 +- >> xen/include/xsm/dummy.h | 6 -- >> xen/include/xsm/xsm.h | 7 --- >> xen/xsm/flask/hooks.c | 6 -- >> 5 files changed, 14 insertions(+), 9 deletions(-) >> >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >> index a810a056d7..9ec78d8c03 100644 >> --- a/xen/arch/arm/mm.c >> +++ b/xen/arch/arm/mm.c >> @@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one( >> return -EINVAL; >> } >> >> -rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od); >> +rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od); >> if ( rc ) >> { >> rcu_unlock_domain(od); >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >> index e8a57d118c..a547fd00c0 100644 >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long >> fgfn, >> if ( tdom == fdom ) >> goto out; >> >> -rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom); >> +rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom); >> if ( rc ) >> goto out; >> >> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h >> index 62fcea6f04..28dbc6f2a2 100644 >> --- 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); > > We need to preserve the returned errors: > > rc = xsm_default_action(action, cd, d); > if (rc) return rc; > return xsm_default_action(action, cd, t); OK, will correct this. > > > >> } >> >> static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, >> unsigned long op) >> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h >> index 60c0fd6a62..a20654a803 100644 >> --- a/xen/include/xsm/xsm.h >> +++ b/xen/include/xsm/xsm.h >> @@ -85,7 +85,7 @@ struct xsm_operations { >> int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct >> page_info *page); >> int (*add_to_physmap) (struct domain *d1, struct domain *d2); >> int (*remove_from_physmap) (struct domain *d1, struct domain *d2); >> -int (*map_gmfn_foreign) (struct domain *d, struct domain *t); >> +int (*map_gmfn_foreign) (struct domain *cd, struct domain *d, struct >> domain *t); >> int (*claim_pages) (struct domain *d); >> >> int (*console_io) (struct domain *d, int cmd); >> @@ -372,9 +372,10 @@ static inline int xsm_remove_from_physmap(xsm_default_t >> def, struct domain *d1, >> return xsm_ops->remove_from_physmap(d1, d2); >> } >> >> -static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain >> *d, struct domain *t) >> +static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain >> *cd, >> +struct domain *d, struct domain *t) >> { >> -return xsm_ops->map_gmfn_foreign(d, t); >> +return xsm_ops->map_gmfn_foreign(cd, d, t); >> } >> >> static inline int xsm_claim_pages(xsm_default_t def, struct
Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
On Wed, 23 Aug 2017, Zhongze Liu 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 ^ IFF > target domain and source domain, grant the access. > > References to this xsm check have also been updated to pass the current > domain as a new parameter. > > This is for the proposal "Allow setting up shared memory areas between VMs > from xl config file" (see [1]). > > [1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html > > Signed-off-by: Zhongze Liu> > Cc: Stefano Stabellini > Cc: Julien Grall > Cc: George Dunlap > Cc: Jan Beulich > Cc: Andrew Cooper > Cc: Daniel De Graaf > Cc: xen-devel@lists.xen.org > --- > xen/arch/arm/mm.c | 2 +- > xen/arch/x86/mm/p2m.c | 2 +- > xen/include/xsm/dummy.h | 6 -- > xen/include/xsm/xsm.h | 7 --- > xen/xsm/flask/hooks.c | 6 -- > 5 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index a810a056d7..9ec78d8c03 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one( > return -EINVAL; > } > > -rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od); > +rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od); > if ( rc ) > { > rcu_unlock_domain(od); > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index e8a57d118c..a547fd00c0 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long > fgfn, > if ( tdom == fdom ) > goto out; > > -rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom); > +rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom); > if ( rc ) > goto out; > > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index 62fcea6f04..28dbc6f2a2 100644 > --- 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); We need to preserve the returned errors: rc = xsm_default_action(action, cd, d); if (rc) return rc; return xsm_default_action(action, cd, t); > } > > static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, > unsigned long op) > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h > index 60c0fd6a62..a20654a803 100644 > --- a/xen/include/xsm/xsm.h > +++ b/xen/include/xsm/xsm.h > @@ -85,7 +85,7 @@ struct xsm_operations { > int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct > page_info *page); > int (*add_to_physmap) (struct domain *d1, struct domain *d2); > int (*remove_from_physmap) (struct domain *d1, struct domain *d2); > -int (*map_gmfn_foreign) (struct domain *d, struct domain *t); > +int (*map_gmfn_foreign) (struct domain *cd, struct domain *d, struct > domain *t); > int (*claim_pages) (struct domain *d); > > int (*console_io) (struct domain *d, int cmd); > @@ -372,9 +372,10 @@ static inline int xsm_remove_from_physmap(xsm_default_t > def, struct domain *d1, > return xsm_ops->remove_from_physmap(d1, d2); > } > > -static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *d, > struct domain *t) > +static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *cd, > +struct domain *d, struct domain *t) > { > -return xsm_ops->map_gmfn_foreign(d, t); > +return xsm_ops->map_gmfn_foreign(cd, d, t); > } > > static inline int xsm_claim_pages(xsm_default_t def, struct domain *d) > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index 91146275bb..3408b6b9e1 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct
[Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
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. References to this xsm check have also been updated to pass the current domain as a new parameter. This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]). [1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html Signed-off-by: Zhongze LiuCc: Stefano Stabellini Cc: Julien Grall Cc: George Dunlap Cc: Jan Beulich Cc: Andrew Cooper Cc: Daniel De Graaf Cc: xen-devel@lists.xen.org --- xen/arch/arm/mm.c | 2 +- xen/arch/x86/mm/p2m.c | 2 +- xen/include/xsm/dummy.h | 6 -- xen/include/xsm/xsm.h | 7 --- xen/xsm/flask/hooks.c | 6 -- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index a810a056d7..9ec78d8c03 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one( return -EINVAL; } -rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od); +rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od); if ( rc ) { rcu_unlock_domain(od); diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index e8a57d118c..a547fd00c0 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, if ( tdom == fdom ) goto out; -rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom); +rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom); if ( rc ) goto out; diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 62fcea6f04..28dbc6f2a2 100644 --- 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); } static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op) diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 60c0fd6a62..a20654a803 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -85,7 +85,7 @@ struct xsm_operations { int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct page_info *page); int (*add_to_physmap) (struct domain *d1, struct domain *d2); int (*remove_from_physmap) (struct domain *d1, struct domain *d2); -int (*map_gmfn_foreign) (struct domain *d, struct domain *t); +int (*map_gmfn_foreign) (struct domain *cd, struct domain *d, struct domain *t); int (*claim_pages) (struct domain *d); int (*console_io) (struct domain *d, int cmd); @@ -372,9 +372,10 @@ static inline int xsm_remove_from_physmap(xsm_default_t def, struct domain *d1, return xsm_ops->remove_from_physmap(d1, d2); } -static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *d, struct domain *t) +static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *cd, +struct domain *d, struct domain *t) { -return xsm_ops->map_gmfn_foreign(d, t); +return xsm_ops->map_gmfn_foreign(cd, d, t); } static inline int xsm_claim_pages(xsm_default_t def, struct domain *d) diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 91146275bb..3408b6b9e1 100644 --- 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) || +