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

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Hi Stefano,

2017-08-23 3:58 GMT+08:00 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);

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

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