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

[Jan Beulich]

>>> On 25.10.17 at 11:37,  wrote:
> My current plan is to add the following new MAPSPACE to public/memory.h:
> 
> +#define XENMEMSPACE_gmfn_foreign_share 6 /* Same as *_gmfn_foreign, but this 
> is
> +for a privileged dom to
> +shared pages between two doms. */

I think XENMEMSPACE_gmfn_share would suffice as a name.

Jan


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

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

[Zhongze Liu]

2017-10-25 17:37 GMT+08:00 Zhongze Liu :
> Hi,
>
> My current plan is to add the following new MAPSPACE to public/memory.h:
>
> +#define XENMEMSPACE_gmfn_foreign_share 6 /* Same as *_gmfn_foreign, but this 
> is
> +for a privileged dom to
> +shared pages between two doms. */

s/shared/share

>
> and create a corresponding  entry xsm_map_gmfn_foreign_share to the
> xsm structure, which will be filled with
> the proposed policy.
>
> Does this look good to you?
>
> Cheers,
>
> Zhongze Liu

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

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

[Zhongze Liu]

Hi,

My current plan is to add the following new MAPSPACE to public/memory.h:

+#define XENMEMSPACE_gmfn_foreign_share 6 /* Same as *_gmfn_foreign, but this is
+for a privileged dom to
+shared pages between two doms. */

and create a corresponding  entry xsm_map_gmfn_foreign_share to the
xsm structure, which will be filled with
the proposed policy.

Does this look good to you?

Cheers,

Zhongze Liu

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

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

[Zhongze Liu]

Hi Jan,

2017-10-23 15:26 GMT+08:00 Jan Beulich :
 On 22.10.17 at 13:21,  wrote:
>> How about changing the policy to (c over d) && ((d over t) || (c over t))?
>> Given that (c over d) is a must, which is always checked somewhere higher
>> in the call stack as Daniel pointed out,  permitting (d over t) or (c
>> over t) actually infers
>> permitting the other.
>>
>> - if you permit (d over t) but not (c over t):
>>   Given (c over t),
>>   (c) can first map the src page from (t) into its own memory space and then 
>> map
>>   this page from its own memory space to (d)'s memory space.
>
> Would that work? The page, when in (c)'s space, is still owned by (t),
> so I don't see how mapping into (d)'s space could become possible
> just because it's mapped into (c)'s.

Yes, indeed. This won't work. Sorry for giving a wrong example here.

I think I now agree to add a new subop, too.

Cheers,

Zhongze Liu

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

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

[Jan Beulich]

>>> On 22.10.17 at 13:21,  wrote:
> How about changing the policy to (c over d) && ((d over t) || (c over t))?
> Given that (c over d) is a must, which is always checked somewhere higher
> in the call stack as Daniel pointed out,  permitting (d over t) or (c
> over t) actually infers
> permitting the other.
> 
> - if you permit (d over t) but not (c over t):
>   Given (c over t),
>   (c) can first map the src page from (t) into its own memory space and then 
> map
>   this page from its own memory space to (d)'s memory space.

Would that work? The page, when in (c)'s space, is still owned by (t),
so I don't see how mapping into (d)'s space could become possible
just because it's mapped into (c)'s.

> - if you permit (c over t) but not (d over t):
>   Given (d over t),
>   (c) can first map (d)'s pages into its own memory space and modify (d)'s 
> code
>   to issues a hypercall that maps (t)'s memory pages into (d)'s memory space.

I can buy this one (after having thought about it a little only for
now), albeit (c) modifying code in (d) is certainly something I'd call
abuse rather than use of permissions.

Jan


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

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

[Zhongze Liu]

Hi Daniel and Jan,

2017-10-20 21:34 GMT+08:00 Daniel De Graaf :
> On 10/20/2017 02:14 AM, Jan Beulich wrote:
>
> On 19.10.17 at 19:36,  wrote:
>>>
>>> On 10/19/2017 07:58 AM, Jan Beulich wrote:
>>>
>>> On 19.10.17 at 04:36,  wrote:
>
> --- 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);
>}


 When all three domains are different, how does the changed
 policy reflect the original "d has privilege over t" requirement?
 I understand you want to relax the current condition, but this
 shouldn't come at the price of granting access when access
 should be denied. Nor the inverse - the current domain not
 having privilege over both does also not mean d doesn't have
 the necessary privilege over t.

 I continue to think that you can't validly retrofit the new
 intended functionality onto the existing hypercall, even if
 nothing except the permission check needs to be different.

 Jan
>>>
>>>
>>> If this operation is going to be allowed at all (and I agree it has
>>> valid use cases), then there won't be a privilege relationship between
>>> (d) and (t) to check - they'll both be (somewhat related) domUs as far
>>> as Xen can tell.  If this hypercall isn't used, adding a new hypercall
>>> (subop) is the only way I'd see to do it - and that seems very redundant
>>> as it'd need to do all the same checks except for the one about the
>>> relationship between (d) and (t).  I don't see the reason why the
>>> existing hypercall should deny being used for that purpose once it's
>>> possible using other means.
>>
>>
>> One problem is, as you mention here, ...
>>
>>> The only possible problem that springs to mind is a restricted kernel
>>> interface (such as the one used by QEMU in dom0 that restricts to a
>>> single target domain) that now doesn't realize it's relaying an
>>> operation that also requires permission over (t) after only checking
>>> that the origin is allowed to modify (d).
>>
>>
>> ... the delegation of privilege checking responsibility to a
>> possibly untrusted environment. Plus, as explained before,
>> current callers expect privilege of d over t to be validated,
>> which isn't happening anymore with the proposed change. If
>> the existing sub-op was to be modified, I think we'd need
>> (with c representing the current domain)
>> - (d over t) || ((c over d) && (c over t)) for not regressing
>>the pre-existing use case,
>> - only (c over d) && (c over t) for not permitting something
>>that isn't intended to be permitted in the new use case.
>> Unless the sub-op has room for adding a flag to indicate
>> which of the two is meant (I didn't check), I don't see a way
>> around adding another sub-op, no matter how similar this
>> would end up being.
>>
>> Jan
>
>
> I would say the current lack of a check for (c over t) is an oversight,
> which mostly doesn't matter because the ability to modify arbitrary
> memory in your target is transitive in almost any security model (c can
> modify d's code to modify t, so a malicious c can compromise t anyway).
> If the three domains are all different, the only way this can happen in
> non-XSM is for (c) to be dom0 or for your device model to have a device
> model (which I don't think is forbidden, but doubt anyone uses).
>
> I now agree that this deserves a new subop, since this code is reached
> via the stable memory_op and not just a domctl.

How about changing the policy to (c over d) && ((d over t) || (c over t))?
Given that (c over d) is a must, which is always checked somewhere higher
in the call stack as Daniel pointed out,  permitting (d over t) or (c
over t) actually infers
permitting the other.

- if you permit (d over t) but not (c over t):
  Given (c over t),
  (c) can first map the src page from (t) into its own memory space and then map
  this page from its own memory space to (d)'s memory space.

- if you permit (c over t) but not (d over t):
  Given (d over t),
  (c) can first map (d)'s pages into its own memory space and modify (d)'s code
  to issues a hypercall that maps (t)'s memory pages into (d)'s memory space.

I'm not very familiar with Xen's security model. So I might be totally
wrong here.
If so, please correct me.

And if you still think adding a new subop is necessary, do you have
any suggestions
on this?

Cheers,

Zhongze Liu

___
Xen-devel mailing 

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

[Daniel De Graaf]

On 10/20/2017 02:14 AM, Jan Beulich wrote:

On 19.10.17 at 19:36,  wrote:

On 10/19/2017 07:58 AM, Jan Beulich wrote:

On 19.10.17 at 04:36,  wrote:

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


When all three domains are different, how does the changed
policy reflect the original "d has privilege over t" requirement?
I understand you want to relax the current condition, but this
shouldn't come at the price of granting access when access
should be denied. Nor the inverse - the current domain not
having privilege over both does also not mean d doesn't have
the necessary privilege over t.

I continue to think that you can't validly retrofit the new
intended functionality onto the existing hypercall, even if
nothing except the permission check needs to be different.

Jan


If this operation is going to be allowed at all (and I agree it has
valid use cases), then there won't be a privilege relationship between
(d) and (t) to check - they'll both be (somewhat related) domUs as far
as Xen can tell.  If this hypercall isn't used, adding a new hypercall
(subop) is the only way I'd see to do it - and that seems very redundant
as it'd need to do all the same checks except for the one about the
relationship between (d) and (t).  I don't see the reason why the
existing hypercall should deny being used for that purpose once it's
possible using other means.


One problem is, as you mention here, ...


The only possible problem that springs to mind is a restricted kernel
interface (such as the one used by QEMU in dom0 that restricts to a
single target domain) that now doesn't realize it's relaying an
operation that also requires permission over (t) after only checking
that the origin is allowed to modify (d).


... the delegation of privilege checking responsibility to a
possibly untrusted environment. Plus, as explained before,
current callers expect privilege of d over t to be validated,
which isn't happening anymore with the proposed change. If
the existing sub-op was to be modified, I think we'd need
(with c representing the current domain)
- (d over t) || ((c over d) && (c over t)) for not regressing
   the pre-existing use case,
- only (c over d) && (c over t) for not permitting something
   that isn't intended to be permitted in the new use case.
Unless the sub-op has room for adding a flag to indicate
which of the two is meant (I didn't check), I don't see a way
around adding another sub-op, no matter how similar this
would end up being.

Jan


I would say the current lack of a check for (c over t) is an oversight,
which mostly doesn't matter because the ability to modify arbitrary
memory in your target is transitive in almost any security model (c can
modify d's code to modify t, so a malicious c can compromise t anyway).
If the three domains are all different, the only way this can happen in
non-XSM is for (c) to be dom0 or for your device model to have a device
model (which I don't think is forbidden, but doubt anyone uses).

I now agree that this deserves a new subop, since this code is reached
via the stable memory_op and not just a domctl.

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

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

[Daniel De Graaf]

On 10/19/2017 08:55 PM, Zhongze Liu wrote:

2017-10-20 8:34 GMT+08:00 Zhongze Liu :

Hi Daniel,

2017-10-20 1:36 GMT+08:00 Daniel De Graaf :

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 

Cc: Daniel De Graaf 
Cc: Ian Jackson 
Cc: Wei Liu 
Cc: Stefano Stabellini 
Cc: Julien Grall 
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?


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.


If this were not the case, there 

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

[Jan Beulich]

>>> On 19.10.17 at 19:36,  wrote:
> On 10/19/2017 07:58 AM, Jan Beulich wrote:
> On 19.10.17 at 04:36,  wrote:
>>> --- 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);
>>>   }
>> 
>> When all three domains are different, how does the changed
>> policy reflect the original "d has privilege over t" requirement?
>> I understand you want to relax the current condition, but this
>> shouldn't come at the price of granting access when access
>> should be denied. Nor the inverse - the current domain not
>> having privilege over both does also not mean d doesn't have
>> the necessary privilege over t.
>> 
>> I continue to think that you can't validly retrofit the new
>> intended functionality onto the existing hypercall, even if
>> nothing except the permission check needs to be different.
>> 
>> Jan
> 
> If this operation is going to be allowed at all (and I agree it has
> valid use cases), then there won't be a privilege relationship between
> (d) and (t) to check - they'll both be (somewhat related) domUs as far
> as Xen can tell.  If this hypercall isn't used, adding a new hypercall
> (subop) is the only way I'd see to do it - and that seems very redundant
> as it'd need to do all the same checks except for the one about the
> relationship between (d) and (t).  I don't see the reason why the
> existing hypercall should deny being used for that purpose once it's
> possible using other means.

One problem is, as you mention here, ...

> The only possible problem that springs to mind is a restricted kernel
> interface (such as the one used by QEMU in dom0 that restricts to a
> single target domain) that now doesn't realize it's relaying an
> operation that also requires permission over (t) after only checking
> that the origin is allowed to modify (d).

... the delegation of privilege checking responsibility to a
possibly untrusted environment. Plus, as explained before,
current callers expect privilege of d over t to be validated,
which isn't happening anymore with the proposed change. If
the existing sub-op was to be modified, I think we'd need
(with c representing the current domain)
- (d over t) || ((c over d) && (c over t)) for not regressing
  the pre-existing use case,
- only (c over d) && (c over t) for not permitting something
  that isn't intended to be permitted in the new use case.
Unless the sub-op has room for adding a flag to indicate
which of the two is meant (I didn't check), I don't see a way
around adding another sub-op, no matter how similar this
would end up being.

Jan


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

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

[Zhongze Liu]

2017-10-20 8:34 GMT+08:00 Zhongze Liu :
> Hi Daniel,
>
> 2017-10-20 1:36 GMT+08:00 Daniel De Graaf :
>> 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 
>>>
>>> Cc: Daniel De Graaf 
>>> Cc: Ian Jackson 
>>> Cc: Wei Liu 
>>> Cc: Stefano Stabellini 
>>> Cc: Julien Grall 
>>> 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 

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

[Zhongze Liu]

Hi Daniel,

2017-10-20 1:36 GMT+08:00 Daniel De Graaf :
> 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 
>>
>> Cc: Daniel De Graaf 
>> Cc: Ian Jackson 
>> Cc: Wei Liu 
>> Cc: Stefano Stabellini 
>> Cc: Julien Grall 
>> 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

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

[Daniel De Graaf]

On 10/19/2017 07:58 AM, Jan Beulich wrote:

On 19.10.17 at 04:36,  wrote:

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


When all three domains are different, how does the changed
policy reflect the original "d has privilege over t" requirement?
I understand you want to relax the current condition, but this
shouldn't come at the price of granting access when access
should be denied. Nor the inverse - the current domain not
having privilege over both does also not mean d doesn't have
the necessary privilege over t.

I continue to think that you can't validly retrofit the new
intended functionality onto the existing hypercall, even if
nothing except the permission check needs to be different.

Jan


If this operation is going to be allowed at all (and I agree it has
valid use cases), then there won't be a privilege relationship between
(d) and (t) to check - they'll both be (somewhat related) domUs as far
as Xen can tell.  If this hypercall isn't used, adding a new hypercall
(subop) is the only way I'd see to do it - and that seems very redundant
as it'd need to do all the same checks except for the one about the
relationship between (d) and (t).  I don't see the reason why the
existing hypercall should deny being used for that purpose once it's
possible using other means.

The only possible problem that springs to mind is a restricted kernel
interface (such as the one used by QEMU in dom0 that restricts to a
single target domain) that now doesn't realize it's relaying an
operation that also requires permission over (t) after only checking
that the origin is allowed to modify (d).

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

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

[Daniel De Graaf]

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 

Cc: Daniel De Graaf 
Cc: Ian Jackson 
Cc: Wei Liu 
Cc: Stefano Stabellini 
Cc: Julien Grall 
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.

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

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

[Jan Beulich]

>>> On 19.10.17 at 04:36,  wrote:
> --- 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);
>  }

When all three domains are different, how does the changed
policy reflect the original "d has privilege over t" requirement?
I understand you want to relax the current condition, but this
shouldn't come at the price of granting access when access
should be denied. Nor the inverse - the current domain not
having privilege over both does also not mean d doesn't have
the necessary privilege over t.

I continue to think that you can't validly retrofit the new
intended functionality onto the existing hypercall, even if
nothing except the permission check needs to be different.

Jan


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

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

[Zhongze Liu]

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 

Cc: Daniel De Graaf 
Cc: Ian Jackson 
Cc: Wei Liu 
Cc: Stefano Stabellini 
Cc: Julien Grall 
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);
 }
 
 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);
 }
 
 static int flask_hvm_param(struct domain *d, unsigned long op)
diff --git a/xen/xsm/flask/policy/access_vectors 
b/xen/xsm/flask/policy/access_vectors
index 3a2d863b8f..a0330f914a 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -387,6 +387,10 @@ class mmu
 # Allow a privileged domain to install a map of a page it does not own.  Used
 # for stub domain device models with the PV framebuffer.
 target_hack
+# Checked when using map_gmfn_foreign to share memory:
+#  source = domain whose memory is being shared
+#  target = client domain
+share_mem
 }
 
 # control of the paging_domctl split by subop
-- 
2.14.2


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