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

2017-08-24 Thread Daniel De Graaf 

On 08/24/2017 08:39 AM, Jan Beulich wrote:

On 24.08.17 at 13:33,  wrote:

Hi Jan,

2017-08-24 14:37 GMT+08:00 Jan Beulich :

On 24.08.17 at 02:51,  wrote:

2017-08-23 17:55 GMT+08:00 Jan Beulich :

On 22.08.17 at 20:08,  wrote:

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


... you use "or" here and ...


This might be confusing. But think of returning 0 as "allowed", the
only condition where this
statement returns a 0 is when both calls return 0 -- so it's actually
an "and". (Think of de-morgan's law.)

But as Stefano has pointed out, I should preserve the error code.


Ah, right - the code as written suggests boolean return values,
which gives it the wrong look. You really mean ?: instead of ||.
Why do you, btw, pass in current->domain (as cd) instead of
obtaining it here (just like various other hooks do)?


This was my original plan, but I'm not very sure about this, so I turn
to Julien for help, and...
Here is part of the irc log from a discussion with Julien on
#xendevel, where Julien said:

blackskygg: I think you want to pass the current domain in
parameter, i.e having 3 domains argument.
because your solution only works when XSM is not enabled (this is
the dummy callback)
when XSM is enabled, the policy would be specificed by the administrator
he needs to be able to know which domain was doing the configuration.


in flask/hooks.c there are quite a few uses of
avc_current_has_perm() in such cases, so I would think not
handing current->domain through the hook should be fine. But
of course Daniel may tell you I'm completely wrong here.

Jan


This is really just a choice of whatever looks better.  There's a very minor
performance penalty from not calling current->domain over and over, but there
might also be a performance gain if current_has_perm is not inlined and the
call results in smaller code size.

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

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

2017-08-24 Thread Jan Beulich 
>>> On 24.08.17 at 13:33,  wrote:
> Hi Jan,
> 
> 2017-08-24 14:37 GMT+08:00 Jan Beulich :
> On 24.08.17 at 02:51,  wrote:
>>> 2017-08-23 17:55 GMT+08:00 Jan Beulich :
>>> On 22.08.17 at 20:08,  wrote:
> --- 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);
>  }

 ... you use "or" here and ...
>>>
>>> This might be confusing. But think of returning 0 as "allowed", the
>>> only condition where this
>>> statement returns a 0 is when both calls return 0 -- so it's actually
>>> an "and". (Think of de-morgan's law.)
>>>
>>> But as Stefano has pointed out, I should preserve the error code.
>>
>> Ah, right - the code as written suggests boolean return values,
>> which gives it the wrong look. You really mean ?: instead of ||.
>> Why do you, btw, pass in current->domain (as cd) instead of
>> obtaining it here (just like various other hooks do)?
> 
> This was my original plan, but I'm not very sure about this, so I turn
> to Julien for help, and...
> Here is part of the irc log from a discussion with Julien on
> #xendevel, where Julien said:
> 
>blackskygg: I think you want to pass the current domain in
> parameter, i.e having 3 domains argument.
>because your solution only works when XSM is not enabled (this is
> the dummy callback)
>when XSM is enabled, the policy would be specificed by the administrator
>he needs to be able to know which domain was doing the configuration.

in flask/hooks.c there are quite a few uses of
avc_current_has_perm() in such cases, so I would think not
handing current->domain through the hook should be fine. But
of course Daniel may tell you I'm completely wrong here.

Jan


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

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

2017-08-24 Thread Zhongze Liu 
Hi Jan,

2017-08-24 14:37 GMT+08:00 Jan Beulich :
 On 24.08.17 at 02:51,  wrote:
>> 2017-08-23 17:55 GMT+08:00 Jan Beulich :
>> On 22.08.17 at 20:08,  wrote:
 --- 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);
  }
>>>
>>> ... you use "or" here and ...
>>
>> This might be confusing. But think of returning 0 as "allowed", the
>> only condition where this
>> statement returns a 0 is when both calls return 0 -- so it's actually
>> an "and". (Think of de-morgan's law.)
>>
>> But as Stefano has pointed out, I should preserve the error code.
>
> Ah, right - the code as written suggests boolean return values,
> which gives it the wrong look. You really mean ?: instead of ||.
> Why do you, btw, pass in current->domain (as cd) instead of
> obtaining it here (just like various other hooks do)?

This was my original plan, but I'm not very sure about this, so I turn
to Julien for help, and...
Here is part of the irc log from a discussion with Julien on
#xendevel, where Julien said:

   blackskygg: I think you want to pass the current domain in
parameter, i.e having 3 domains argument.
   because your solution only works when XSM is not enabled (this is
the dummy callback)
   when XSM is enabled, the policy would be specificed by the administrator
   he needs to be able to know which domain was doing the configuration.

Cheers,

Zhongze Liu
>
> Jan
>

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

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

2017-08-24 Thread Jan Beulich 
>>> On 24.08.17 at 02:51,  wrote:
> 2017-08-23 17:55 GMT+08:00 Jan Beulich :
> On 22.08.17 at 20:08,  wrote:
>>> --- 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);
>>>  }
>>
>> ... you use "or" here and ...
> 
> This might be confusing. But think of returning 0 as "allowed", the
> only condition where this
> statement returns a 0 is when both calls return 0 -- so it's actually
> an "and". (Think of de-morgan's law.)
> 
> But as Stefano has pointed out, I should preserve the error code.

Ah, right - the code as written suggests boolean return values,
which gives it the wrong look. You really mean ?: instead of ||.
Why do you, btw, pass in current->domain (as cd) instead of
obtaining it here (just like various other hooks do)?

Jan


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

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

2017-08-24 Thread Jan Beulich 
>>> On 23.08.17 at 19:16,  wrote:
> On Wed, 23 Aug 2017, Jan Beulich wrote:
>> >>> On 22.08.17 at 20:08,  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
>> > target domain and source domain, grant the access.
>> 
>> You say "and here", yet ...
>> 
>> > --- 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);
>> >  }
>> 
>> ... you use "or" here and ...
>> 
>> > --- 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);
>> >  }
>> 
>> ... here. A domain can't have XSM_TARGET permission over two
>> other domains, so what you want to do here can't work at all,
>> afaict.
> 
> It would work with XSM_TARGET if cd == d, and cd has XSM_TARGET
> permission over t (current case). Otherwise, it would work if cd is
> XSM_PRIV (Zhongze's case). Did I get it wrong?

I think so, yes, but besides the "and" vs "or" discrepancy the patch
description suggests a three-way operation (i.e. including the case
of all three domains being different ones). The case you describe
doesn't require three domains to be passed into the hook, it would
- afaict - just be the traditional "cd is XSM_TARGET over t" case
(matching for example xsm_evtchn_{status,unbound}()).

Jan


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

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

2017-08-23 Thread Zhongze Liu 
Hi Jan,

Thanks for reviewing my patch.

2017-08-23 17:55 GMT+08:00 Jan Beulich :
 On 22.08.17 at 20:08,  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
>> target domain and source domain, grant the access.
>
> You say "and here", yet ...
>
>> --- 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);
>>  }
>
> ... you use "or" here and ...

This might be confusing. But think of returning 0 as "allowed", the
only condition where this
statement returns a 0 is when both calls return 0 -- so it's actually
an "and". (Think of de-morgan's law.)

But as Stefano has pointed out, I should preserve the error code.
And as Daniel has pointed out, I should also check if d and t can share memory.

>
>> --- 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);
>>  }
>
> ... here. A domain can't have XSM_TARGET permission over two
> other domains, so what you want to do here can't work at all,
> afaict.

I agree with what Stefano has said below.

Cheers,

Zhongze Liu.

>
> Jan
>

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

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

2017-08-23 Thread Stefano Stabellini 
On Wed, 23 Aug 2017, Jan Beulich wrote:
> >>> On 22.08.17 at 20:08,  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
> > target domain and source domain, grant the access.
> 
> You say "and here", yet ...
> 
> > --- 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);
> >  }
> 
> ... you use "or" here and ...
> 
> > --- 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);
> >  }
> 
> ... here. A domain can't have XSM_TARGET permission over two
> other domains, so what you want to do here can't work at all,
> afaict.

It would work with XSM_TARGET if cd == d, and cd has XSM_TARGET
permission over t (current case). Otherwise, it would work if cd is
XSM_PRIV (Zhongze's case). Did I get it wrong?

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

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

2017-08-23 Thread Daniel De Graaf 

On 08/22/2017 10:18 PM, Zhongze Liu wrote:

Hi Stefano,

2017-08-23 3:58 GMT+08:00 Stefano Stabellini :

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 

Cc: Stefano Stabellini 
Cc: Julien Grall 
Cc: George Dunlap 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Daniel De Graaf 
Cc: xen-devel@lists.xen.org
---
  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

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

2017-08-23 Thread Jan Beulich 
>>> On 22.08.17 at 20:08,  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
> target domain and source domain, grant the access.

You say "and here", yet ...

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

... you use "or" here and ...

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

... here. A domain can't have XSM_TARGET permission over two
other domains, so what you want to do here can't work at all,
afaict.

Jan


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

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

2017-08-22 Thread Zhongze Liu 
Hi Stefano,

2017-08-23 3:58 GMT+08:00 Stefano Stabellini :
> 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 
>>
>> Cc: Stefano Stabellini 
>> Cc: Julien Grall 
>> Cc: George Dunlap 
>> Cc: Jan Beulich 
>> Cc: Andrew Cooper 
>> Cc: Daniel De Graaf 
>> Cc: xen-devel@lists.xen.org
>> ---
>>  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

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

2017-08-22 Thread Stefano Stabellini 
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 
> 
> Cc: Stefano Stabellini 
> Cc: Julien Grall 
> Cc: George Dunlap 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> Cc: Daniel De Graaf 
> Cc: xen-devel@lists.xen.org
> ---
>  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);



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

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

2017-08-22 Thread Zhongze Liu 
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
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 

Cc: Stefano Stabellini 
Cc: Julien Grall 
Cc: George Dunlap 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Daniel De Graaf 
Cc: xen-devel@lists.xen.org
---
 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);
 }
 
 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) ||
+