On 08/22/2017 10:18 PM, Zhongze Liu wrote:
Hi Stefano,2017-08-23 3:58 GMT+08:00 Stefano Stabellini <sstabell...@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^ IFFtarget 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 <blacksk...@gmail.com> Cc: Stefano Stabellini <sstabell...@kernel.org> Cc: Julien Grall <julien.gr...@arm.com> Cc: George Dunlap <george.dun...@eu.citrix.com> Cc: Jan Beulich <jbeul...@suse.com> Cc: Andrew Cooper <andrew.coop...@citrix.com> Cc: Daniel De Graaf <dgde...@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.
While this permission check with (cd == d) should succeed in all sane policies,
it's faster to compare for equality than to look up the access vector.
In addition, I think it would be useful to have a check that (d) and (t) can
share memory (so that a security policy could be written preventing them from
communicating directly). Normally, this would be allowed between all domains
that allow grant mapping/event channels.
rc = domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
if (rc) return rc;
To allow this in the policy the same as grants:
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -127,6 +127,8 @@ define(`domain_comms', `
domain_event_comms($1, $2)
allow $1 $2:grant { map_read map_write copy unmap };
allow $2 $1:grant { map_read map_write copy unmap };
+ allow $1 $2:mmu share_mem;
+ allow $2 $1:mmu share_mem;
')
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
