| Message ID | 20170822180840.20981-5-blackskygg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <blackskygg@gmail.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> > 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 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); > } Same here: rc = domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE); if (rc) return rc; return domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE); Also, I just want to point out that in the regular case cd and d are one and the same. The code assumes that domain_has_perm returns 0 in that case. I think that is correct, but I don't know enough about XSM to be sure about it.
Hi Stefano, 2017-08-23 3:58 GMT+08:00 Stefano Stabellini <sstabellini@kernel.org>: > 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 <blackskygg@gmail.com> >> >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> Cc: George Dunlap <george.dunlap@eu.citrix.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> 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 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); >> } > > Same here: > > rc = domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE); > if (rc) return rc; > return domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE); > > Also, I just want to point out that in the regular case cd and d are one > and the same. The code assumes that domain_has_perm returns 0 in that > case. I think that is correct, but I don't know enough about XSM to be > sure about it. I also assume that domain_has_perm returns 0 when cd == d, but let's wait for other maintainers' comments. Cheers, Zhongze Liu
>>> On 22.08.17 at 20:08, <blackskygg@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 ... > --- 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
On Wed, 23 Aug 2017, Jan Beulich wrote: > >>> On 22.08.17 at 20:08, <blackskygg@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 ... > > > --- 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?
Hi Jan, Thanks for reviewing my patch. 2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>> On 22.08.17 at 20:08, <blackskygg@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 >
>>> On 23.08.17 at 19:16, <sstabellini@kernel.org> wrote: > On Wed, 23 Aug 2017, Jan Beulich wrote: >> >>> On 22.08.17 at 20:08, <blackskygg@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 ... >> >> > --- 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
>>> On 24.08.17 at 02:51, <blackskygg@gmail.com> wrote: > 2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> 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
Hi Jan, 2017-08-24 14:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>> On 24.08.17 at 02:51, <blackskygg@gmail.com> wrote: >> 2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> 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 >
>>> On 24.08.17 at 13:33, <blackskygg@gmail.com> wrote: > Hi Jan, > > 2017-08-24 14:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>>> On 24.08.17 at 02:51, <blackskygg@gmail.com> wrote: >>> 2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>>>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> 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
On 08/24/2017 08:39 AM, Jan Beulich wrote: >>>> On 24.08.17 at 13:33, <blackskygg@gmail.com> wrote: >> Hi Jan, >> >> 2017-08-24 14:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>>>> On 24.08.17 at 02:51, <blackskygg@gmail.com> wrote: >>>> 2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>: >>>>>>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> 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.
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) || + domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE); } static int flask_hvm_param(struct domain *d, unsigned long op)
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 Liu <blackskygg@gmail.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> 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(-)