Re: [Xen-devel] [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin

[Zhongze Liu <blackskygg@xxxxxxxxx>]

2017-10-20 8:34 GMT+08:00 Zhongze Liu <blackskygg@xxxxxxxxx>:
> Hi Daniel,
>
> 2017-10-20 1:36 GMT+08:00 Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>:
>> On 10/18/2017 10:36 PM, Zhongze Liu wrote:
>>>
>>> The original dummy xsm_map_gmfn_foregin 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, which restricts some useful
>>> yet not
>>> dangerous use cases of the API, such as sharing pages among DomU's by
>>> calling
>>> XENMEM_add_to_physmap from Dom0.
>>>
>>> For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current
>>> domain
>>> has the proper privileges on (d) and (t), grant the access.
>>>
>>> For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to
>>> denote if
>>> two domains can share memory through map_gmfn_foregin. 2) Change to hook
>>> to
>>> grant the access IFF the current domain has proper MMU privileges on (d)
>>> and (t),
>>> and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default
>>> xen.te
>>> to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event
>>> channels.
>>>
>>> This is for the proposal "Allow setting up shared memory areas between VMs
>>> from xl config file" (see [1]).
>>>
>>> [1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
>>>
>>> Signed-off-by: Zhongze Liu <blackskygg@xxxxxxxxx>
>>>
>>> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>> Cc: Julien Grall <julien.grall@xxxxxxx>
>>> Cc: xen-devel@xxxxxxxxxxxxx
>>> ---
>>>    V3:
>>>    * Change several if statements to the GCC '... = a ?: b' extention.
>>>    * lookup the current domain in the hooks instead of passing it as an
>>> arg
>>> ---
>>>   tools/flask/policy/modules/xen.if   | 2 ++
>>>   xen/include/xsm/dummy.h             | 3 ++-
>>>   xen/xsm/flask/hooks.c               | 4 +++-
>>>   xen/xsm/flask/policy/access_vectors | 4 ++++
>>>   4 files changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/flask/policy/modules/xen.if
>>> b/tools/flask/policy/modules/xen.if
>>> index 55437496f6..3ffd1c6239 100644
>>> --- 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;
>>>   ')
>>>     # domain_self_comms(domain)
>>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>>> index b2cd56cdc5..65e7060ad5 100644
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -516,7 +516,8 @@ static XSM_INLINE int
>>> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>>>   static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
>>> *d, struct domain *t)
>>>   {
>>>       XSM_ASSERT_ACTION(XSM_TARGET);
>>> -    return xsm_default_action(action, d, t);
>>> +    return xsm_default_action(action, current->domain, d) ?:
>>> +        xsm_default_action(action, current->domain, t);
>>>   }
>>
>>
>> Same comment as below, the check between (current->domain) and (d) should
>> be redundant with one higher up in the call stack.  The check between
>> (current->domain) and (t) should remain, although this *does* result in a
>> relaxing of the existing permission checks on the call as Jan noted.
>>
>>>   static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d,
>>> unsigned long op)
>>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>>> index f01b4cfaaa..16103bafc9 100644
>>> --- a/xen/xsm/flask/hooks.c
>>> +++ b/xen/xsm/flask/hooks.c
>>> @@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain
>>> *d1, struct domain *d2)
>>>     static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
>>>   {
>>> -    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ |
>>> MMU__MAP_WRITE);
>>> +    return domain_has_perm(current->domain, d, SECCLASS_MMU,
>>> MMU__MAP_READ | MMU__MAP_WRITE) ?:
>>> +        domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ |
>>> MMU__MAP_WRITE) ?:
>>> +        domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>>>   }
>>
>>
>> This is at least partially redundant with the higher-level permission checks
>> needed to get to the xenmem_add_* functions (xatp_permission_check call in
>> xen/common/memory.c, for example).  That check already verifies the
>> permission
>> for (current->domain) to modify (d)'s page tables.
>>
>> The other two checks here look correct.
>
> Do you mean that the checks that verify the permission for (current->domain) 
> to
> modify (d)'s page tables have already been done somewhere higher up in the
> call stack so that I can eliminate them in both hooks?

Although xatp_permission_chec() does check (current->domain)'s permission over
(d), I'm not sure if this is the case for all the call paths that
would finally lead to map_gmfn_foregin().
If the answer is yes, I would happily remove the redundant checks.

Cheers,

Zhongze Liu.

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