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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxx>
> 
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxx
> ---
>  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.

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