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

[Zhongze Liu <blackskygg@gmail.com>]

Hi Daniel,

2017-10-20 1:36 GMT+08:00 Daniel De Graaf <dgdegra@tycho.nsa.gov>:
> 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@gmail.com>
>>
>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: xen-devel@lists.xen.org
>> ---
>>    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?


Cheers,

Zhongze Liu.

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