[4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Commit Message

Daniel De Graaf Aug. 23, 2017, 3:56 p.m. UTC

On 08/22/2017 10:18 PM, Zhongze Liu wrote:
> Hi Stefano,
> 
> 2017-08-23 3:58 GMT+08:00 Stefano Stabellini <[e-mail]>:
>> 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 <[e-mail]>
>>>
>>> Cc: Stefano Stabellini <[e-mail]>
>>> Cc: Julien Grall <[e-mail]>
>>> Cc: George Dunlap <[e-mail]>
>>> Cc: Jan Beulich <[e-mail]>
>>> Cc: Andrew Cooper <[e-mail]>
>>> Cc: Daniel De Graaf <[e-mail]>
>>> Cc: [e-mail]
>>> ---
>>>   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:

Patch

--- 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;
  ')