[PATCH v2] domctl: relax getdomaininfo permissions

[PATCH v2] domctl: relax getdomaininfo permissions

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4088 bytes --]

Qemu needs access to this for the domain it controls, both due to it
being used by xc_domain_memory_mapping() (which qemu calls) and the
explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend
permissions to that of any "ordinary" domctl: A domain controlling the
targeted domain can invoke this operation for that target domain (which
is being achieved by no longer passing NULL to xsm_domctl()).

This at once avoids a for_each_domain() loop when the ID of an
existing domain gets passed in.

Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add a comment. Clarify description as to what additional permission
    is being granted.
---
I know there had been an alternative patch suggestion, but that one
doesn't seem have seen a formal submission so far, so here is my
original proposal.

I wonder what good the duplication of the returned domain ID does: I'm
tempted to remove the one in the command-specific structure. Does
anyone have insight into why it was done that way?

I further wonder why we have XSM_OTHER: The respective conversion into
other XSM_* values in xsm/dummy.h could as well move into the callers,
making intentions more obvious when looking at the actual code.

--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -149,7 +149,7 @@ define(`device_model', `
 	create_channel($2, $1, $2_channel)
 	allow $1 $2_channel:event create;
 
-	allow $1 $2_target:domain shutdown;
+	allow $1 $2_target:domain { getdomaininfo shutdown };
 	allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack };
 	allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq };
 ')
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -396,14 +396,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
     switch ( op->cmd )
     {
     case XEN_DOMCTL_createdomain:
-    case XEN_DOMCTL_getdomaininfo:
     case XEN_DOMCTL_test_assign_device:
     case XEN_DOMCTL_gdbsx_guestmemio:
         d = NULL;
         break;
     default:
         d = rcu_lock_domain_by_id(op->domain);
-        if ( d == NULL )
+        if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
             return -ESRCH;
     }
 
@@ -817,14 +816,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     case XEN_DOMCTL_getdomaininfo:
     {
-        domid_t dom = op->domain;
-
-        rcu_read_lock(&domlist_read_lock);
+        domid_t dom = DOMID_INVALID;
 
-        for_each_domain ( d )
-            if ( d->domain_id >= dom )
+        if ( !d )
+        {
+            ret = -EINVAL;
+            if ( op->domain >= DOMID_FIRST_RESERVED )
                 break;
 
+            rcu_read_lock(&domlist_read_lock);
+
+            dom = op->domain;
+            for_each_domain ( d )
+                if ( d->domain_id >= dom )
+                    break;
+        }
+
         ret = -ESRCH;
         if ( d == NULL )
             goto getdomaininfo_out;
@@ -839,6 +846,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         copyback = 1;
 
     getdomaininfo_out:
+        /* When d was non-NULL upon entry, no cleanup is needed. */
+        if ( dom == DOMID_INVALID )
+            break;
+
         rcu_read_unlock(&domlist_read_lock);
         d = NULL;
         break;
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -61,7 +61,12 @@ static always_inline int xsm_default_act
         return 0;
     case XSM_TARGET:
         if ( src == target )
+        {
             return 0;
+    case XSM_XS_PRIV:
+            if ( src->is_xenstore )
+                return 0;
+        }
         /* fall through */
     case XSM_DM_PRIV:
         if ( target && src->target == target )
@@ -71,10 +76,6 @@ static always_inline int xsm_default_act
         if ( src->is_privileged )
             return 0;
         return -EPERM;
-    case XSM_XS_PRIV:
-        if ( src->is_xenstore || src->is_privileged )
-            return 0;
-        return -EPERM;
     default:
         LINKER_BUG_ON(1);
         return -EPERM;



[-- Attachment #2: domctl-relax-getdomaininfo.patch --]
[-- Type: text/plain, Size: 4251 bytes --]

domctl: relax getdomaininfo permissions

Qemu needs access to this for the domain it controls, both due to it
being used by xc_domain_memory_mapping() (which qemu calls) and the
explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend
permissions to that of any "ordinary" domctl: A domain controlling the
targeted domain can invoke this operation for that target domain (which
is being achieved by no longer passing NULL to xsm_domctl()).

This at once avoids a for_each_domain() loop when the ID of an
existing domain gets passed in.

Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add a comment. Clarify description as to what additional permission
    is being granted.
---
I know there had been an alternative patch suggestion, but that one
doesn't seem have seen a formal submission so far, so here is my
original proposal.

I wonder what good the duplication of the returned domain ID does: I'm
tempted to remove the one in the command-specific structure. Does
anyone have insight into why it was done that way?

I further wonder why we have XSM_OTHER: The respective conversion into
other XSM_* values in xsm/dummy.h could as well move into the callers,
making intentions more obvious when looking at the actual code.

--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -149,7 +149,7 @@ define(`device_model', `
 	create_channel($2, $1, $2_channel)
 	allow $1 $2_channel:event create;
 
-	allow $1 $2_target:domain shutdown;
+	allow $1 $2_target:domain { getdomaininfo shutdown };
 	allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack };
 	allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq };
 ')
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -396,14 +396,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
     switch ( op->cmd )
     {
     case XEN_DOMCTL_createdomain:
-    case XEN_DOMCTL_getdomaininfo:
     case XEN_DOMCTL_test_assign_device:
     case XEN_DOMCTL_gdbsx_guestmemio:
         d = NULL;
         break;
     default:
         d = rcu_lock_domain_by_id(op->domain);
-        if ( d == NULL )
+        if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
             return -ESRCH;
     }
 
@@ -817,14 +816,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     case XEN_DOMCTL_getdomaininfo:
     {
-        domid_t dom = op->domain;
-
-        rcu_read_lock(&domlist_read_lock);
+        domid_t dom = DOMID_INVALID;
 
-        for_each_domain ( d )
-            if ( d->domain_id >= dom )
+        if ( !d )
+        {
+            ret = -EINVAL;
+            if ( op->domain >= DOMID_FIRST_RESERVED )
                 break;
 
+            rcu_read_lock(&domlist_read_lock);
+
+            dom = op->domain;
+            for_each_domain ( d )
+                if ( d->domain_id >= dom )
+                    break;
+        }
+
         ret = -ESRCH;
         if ( d == NULL )
             goto getdomaininfo_out;
@@ -839,6 +846,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         copyback = 1;
 
     getdomaininfo_out:
+        /* When d was non-NULL upon entry, no cleanup is needed. */
+        if ( dom == DOMID_INVALID )
+            break;
+
         rcu_read_unlock(&domlist_read_lock);
         d = NULL;
         break;
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -61,7 +61,12 @@ static always_inline int xsm_default_act
         return 0;
     case XSM_TARGET:
         if ( src == target )
+        {
             return 0;
+    case XSM_XS_PRIV:
+            if ( src->is_xenstore )
+                return 0;
+        }
         /* fall through */
     case XSM_DM_PRIV:
         if ( target && src->target == target )
@@ -71,10 +76,6 @@ static always_inline int xsm_default_act
         if ( src->is_privileged )
             return 0;
         return -EPERM;
-    case XSM_XS_PRIV:
-        if ( src->is_xenstore || src->is_privileged )
-            return 0;
-        return -EPERM;
     default:
         LINKER_BUG_ON(1);
         return -EPERM;

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2] domctl: relax getdomaininfo permissions

[Andrew Cooper]

On 05/08/16 12:20, Jan Beulich wrote:
> Qemu needs access to this for the domain it controls, both due to it
> being used by xc_domain_memory_mapping() (which qemu calls) and the
> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend
> permissions to that of any "ordinary" domctl: A domain controlling the
> targeted domain can invoke this operation for that target domain (which
> is being achieved by no longer passing NULL to xsm_domctl()).
>
> This at once avoids a for_each_domain() loop when the ID of an
> existing domain gets passed in.
>
> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Add a comment. Clarify description as to what additional permission
>     is being granted.
> ---
> I know there had been an alternative patch suggestion, but that one
> doesn't seem have seen a formal submission so far, so here is my
> original proposal.
>
> I wonder what good the duplication of the returned domain ID does: I'm
> tempted to remove the one in the command-specific structure. Does
> anyone have insight into why it was done that way?

I wonder whether the first incarnation of this hypercall lacked a domid
field in the returned structure?  It seems like the kind of thing which
would be omitted, until the sysctl list version got introduced.

>
> I further wonder why we have XSM_OTHER: The respective conversion into
> other XSM_* values in xsm/dummy.h could as well move into the callers,
> making intentions more obvious when looking at the actual code.
>
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act
>          return 0;
>      case XSM_TARGET:
>          if ( src == target )
> +        {
>              return 0;
> +    case XSM_XS_PRIV:
> +            if ( src->is_xenstore )
> +                return 0;
> +        }
>          /* fall through */
>      case XSM_DM_PRIV:
>          if ( target && src->target == target )
> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act
>          if ( src->is_privileged )
>              return 0;
>          return -EPERM;
> -    case XSM_XS_PRIV:
> -        if ( src->is_xenstore || src->is_privileged )
> -            return 0;
> -        return -EPERM;
>      default:
>          LINKER_BUG_ON(1);
>          return -EPERM;

What is this change in relation to?  I can't see how it is related to
the XSM changes mentioned in the commit, as that is strictly for the use
of XSM_OTHER.

~Andrew

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

Re: [PATCH v2] domctl: relax getdomaininfo permissions

[Jan Beulich]

>>> On 05.08.16 at 15:10, <andrew.cooper3@citrix.com> wrote:
> On 05/08/16 12:20, Jan Beulich wrote:
>> I wonder what good the duplication of the returned domain ID does: I'm
>> tempted to remove the one in the command-specific structure. Does
>> anyone have insight into why it was done that way?
> 
> I wonder whether the first incarnation of this hypercall lacked a domid
> field in the returned structure?  It seems like the kind of thing which
> would be omitted, until the sysctl list version got introduced.

Oh, good point - that makes clear why the field can't be dropped:
That sysctl would break then.

>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act
>>          return 0;
>>      case XSM_TARGET:
>>          if ( src == target )
>> +        {
>>              return 0;
>> +    case XSM_XS_PRIV:
>> +            if ( src->is_xenstore )
>> +                return 0;
>> +        }
>>          /* fall through */
>>      case XSM_DM_PRIV:
>>          if ( target && src->target == target )
>> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act
>>          if ( src->is_privileged )
>>              return 0;
>>          return -EPERM;
>> -    case XSM_XS_PRIV:
>> -        if ( src->is_xenstore || src->is_privileged )
>> -            return 0;
>> -        return -EPERM;
>>      default:
>>          LINKER_BUG_ON(1);
>>          return -EPERM;
> 
> What is this change in relation to?  I can't see how it is related to
> the XSM changes mentioned in the commit, as that is strictly for the use
> of XSM_OTHER.

I don't see any XSM changes mentioned in the description, there
was only the XSM_OTHER related question outside the description.
Anyway - the change above is what guarantees the XSM_XS_PRIV
check, as invoked by xsm_domctl()'s XEN_DOMCTL_getdomaininfo
case, to fall through into XSM_DM_PRIV - after all that's what the
whole patch is about.

Jan


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

Re: [PATCH v2] domctl: relax getdomaininfo permissions

[Andrew Cooper]

On 05/08/16 14:54, Jan Beulich wrote:
>>>> On 05.08.16 at 15:10, <andrew.cooper3@citrix.com> wrote:
>> On 05/08/16 12:20, Jan Beulich wrote:
>>> I wonder what good the duplication of the returned domain ID does: I'm
>>> tempted to remove the one in the command-specific structure. Does
>>> anyone have insight into why it was done that way?
>> I wonder whether the first incarnation of this hypercall lacked a domid
>> field in the returned structure?  It seems like the kind of thing which
>> would be omitted, until the sysctl list version got introduced.
> Oh, good point - that makes clear why the field can't be dropped:
> That sysctl would break then.

Which domid were you referring to then?

The domid in the xen_domctl_getdomaininfo structure clearly needs to
stay, but the domctl "op->domain = op->u.getdomaininfo.domain;"
needn't.  OTOH, as we need to copy back the entire domctl structure
anyway, it doesn't hurt to keep it.

>
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act
>>>          return 0;
>>>      case XSM_TARGET:
>>>          if ( src == target )
>>> +        {
>>>              return 0;
>>> +    case XSM_XS_PRIV:
>>> +            if ( src->is_xenstore )
>>> +                return 0;
>>> +        }
>>>          /* fall through */
>>>      case XSM_DM_PRIV:
>>>          if ( target && src->target == target )
>>> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act
>>>          if ( src->is_privileged )
>>>              return 0;
>>>          return -EPERM;
>>> -    case XSM_XS_PRIV:
>>> -        if ( src->is_xenstore || src->is_privileged )
>>> -            return 0;
>>> -        return -EPERM;
>>>      default:
>>>          LINKER_BUG_ON(1);
>>>          return -EPERM;
>> What is this change in relation to?  I can't see how it is related to
>> the XSM changes mentioned in the commit, as that is strictly for the use
>> of XSM_OTHER.
> I don't see any XSM changes mentioned in the description, there
> was only the XSM_OTHER related question outside the description.
> Anyway - the change above is what guarantees the XSM_XS_PRIV
> check, as invoked by xsm_domctl()'s XEN_DOMCTL_getdomaininfo
> case, to fall through into XSM_DM_PRIV - after all that's what the
> whole patch is about.

But the patch is about a qemu stubdom, which would be DM_PRIV, not XS_PRIV.

~Andrew

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

Re: [PATCH v2] domctl: relax getdomaininfo permissions

[Jan Beulich]

>>> On 05.08.16 at 19:07, <andrew.cooper3@citrix.com> wrote:
> On 05/08/16 14:54, Jan Beulich wrote:
>>>>> On 05.08.16 at 15:10, <andrew.cooper3@citrix.com> wrote:
>>> On 05/08/16 12:20, Jan Beulich wrote:
>>>> I wonder what good the duplication of the returned domain ID does: I'm
>>>> tempted to remove the one in the command-specific structure. Does
>>>> anyone have insight into why it was done that way?
>>> I wonder whether the first incarnation of this hypercall lacked a domid
>>> field in the returned structure?  It seems like the kind of thing which
>>> would be omitted, until the sysctl list version got introduced.
>> Oh, good point - that makes clear why the field can't be dropped:
>> That sysctl would break then.
> 
> Which domid were you referring to then?
> 
> The domid in the xen_domctl_getdomaininfo structure clearly needs to
> stay, but the domctl "op->domain = op->u.getdomaininfo.domain;"
> needn't.  OTOH, as we need to copy back the entire domctl structure
> anyway, it doesn't hurt to keep it.

The comment was about removal of the field, not just the
assignment. But as you did make obvious, the sysctl side needs
it to stay.

>>>> --- a/xen/include/xsm/dummy.h
>>>> +++ b/xen/include/xsm/dummy.h
>>>> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act
>>>>          return 0;
>>>>      case XSM_TARGET:
>>>>          if ( src == target )
>>>> +        {
>>>>              return 0;
>>>> +    case XSM_XS_PRIV:
>>>> +            if ( src->is_xenstore )
>>>> +                return 0;
>>>> +        }
>>>>          /* fall through */
>>>>      case XSM_DM_PRIV:
>>>>          if ( target && src->target == target )
>>>> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act
>>>>          if ( src->is_privileged )
>>>>              return 0;
>>>>          return -EPERM;
>>>> -    case XSM_XS_PRIV:
>>>> -        if ( src->is_xenstore || src->is_privileged )
>>>> -            return 0;
>>>> -        return -EPERM;
>>>>      default:
>>>>          LINKER_BUG_ON(1);
>>>>          return -EPERM;
>>> What is this change in relation to?  I can't see how it is related to
>>> the XSM changes mentioned in the commit, as that is strictly for the use
>>> of XSM_OTHER.
>> I don't see any XSM changes mentioned in the description, there
>> was only the XSM_OTHER related question outside the description.
>> Anyway - the change above is what guarantees the XSM_XS_PRIV
>> check, as invoked by xsm_domctl()'s XEN_DOMCTL_getdomaininfo
>> case, to fall through into XSM_DM_PRIV - after all that's what the
>> whole patch is about.
> 
> But the patch is about a qemu stubdom, which would be DM_PRIV, not XS_PRIV.

The point of the patch is to _extend_ permissions of this domctl
from XS_PRIV to DM_PRIV.

Jan


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

Re: [PATCH v2] domctl: relax getdomaininfo permissions

[Andrew Cooper]

On 08/08/16 07:12, Jan Beulich wrote:
>>>> On 05.08.16 at 19:07, <andrew.cooper3@citrix.com> wrote:
>> On 05/08/16 14:54, Jan Beulich wrote:
>>>>>> On 05.08.16 at 15:10, <andrew.cooper3@citrix.com> wrote:
>>>> On 05/08/16 12:20, Jan Beulich wrote:
>>>>> I wonder what good the duplication of the returned domain ID does: I'm
>>>>> tempted to remove the one in the command-specific structure. Does
>>>>> anyone have insight into why it was done that way?
>>>> I wonder whether the first incarnation of this hypercall lacked a domid
>>>> field in the returned structure?  It seems like the kind of thing which
>>>> would be omitted, until the sysctl list version got introduced.
>>> Oh, good point - that makes clear why the field can't be dropped:
>>> That sysctl would break then.
>> Which domid were you referring to then?
>>
>> The domid in the xen_domctl_getdomaininfo structure clearly needs to
>> stay, but the domctl "op->domain = op->u.getdomaininfo.domain;"
>> needn't.  OTOH, as we need to copy back the entire domctl structure
>> anyway, it doesn't hurt to keep it.
> The comment was about removal of the field, not just the
> assignment. But as you did make obvious, the sysctl side needs
> it to stay.
>
>>>>> --- a/xen/include/xsm/dummy.h
>>>>> +++ b/xen/include/xsm/dummy.h
>>>>> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act
>>>>>          return 0;
>>>>>      case XSM_TARGET:
>>>>>          if ( src == target )
>>>>> +        {
>>>>>              return 0;
>>>>> +    case XSM_XS_PRIV:
>>>>> +            if ( src->is_xenstore )
>>>>> +                return 0;
>>>>> +        }
>>>>>          /* fall through */
>>>>>      case XSM_DM_PRIV:
>>>>>          if ( target && src->target == target )
>>>>> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act
>>>>>          if ( src->is_privileged )
>>>>>              return 0;
>>>>>          return -EPERM;
>>>>> -    case XSM_XS_PRIV:
>>>>> -        if ( src->is_xenstore || src->is_privileged )
>>>>> -            return 0;
>>>>> -        return -EPERM;
>>>>>      default:
>>>>>          LINKER_BUG_ON(1);
>>>>>          return -EPERM;
>>>> What is this change in relation to?  I can't see how it is related to
>>>> the XSM changes mentioned in the commit, as that is strictly for the use
>>>> of XSM_OTHER.
>>> I don't see any XSM changes mentioned in the description, there
>>> was only the XSM_OTHER related question outside the description.
>>> Anyway - the change above is what guarantees the XSM_XS_PRIV
>>> check, as invoked by xsm_domctl()'s XEN_DOMCTL_getdomaininfo
>>> case, to fall through into XSM_DM_PRIV - after all that's what the
>>> whole patch is about.
>> But the patch is about a qemu stubdom, which would be DM_PRIV, not XS_PRIV.
> The point of the patch is to _extend_ permissions of this domctl
> from XS_PRIV to DM_PRIV.

Aah - and this only exists because of the xsm_domctl() bodge with
XSM_OTHER, which actually makes getdomaininfo protected with XS_PRIV.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2] domctl: relax getdomaininfo permissions

[Jan Beulich]

>>> On 05.08.16 at 13:20, <JBeulich@suse.com> wrote:

Daniel,

I've only now realized that I forgot to Cc you on this v2.

Jan

> Qemu needs access to this for the domain it controls, both due to it
> being used by xc_domain_memory_mapping() (which qemu calls) and the
> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend
> permissions to that of any "ordinary" domctl: A domain controlling the
> targeted domain can invoke this operation for that target domain (which
> is being achieved by no longer passing NULL to xsm_domctl()).
> 
> This at once avoids a for_each_domain() loop when the ID of an
> existing domain gets passed in.
> 
> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Add a comment. Clarify description as to what additional permission
>     is being granted.
> ---
> I know there had been an alternative patch suggestion, but that one
> doesn't seem have seen a formal submission so far, so here is my
> original proposal.
> 
> I wonder what good the duplication of the returned domain ID does: I'm
> tempted to remove the one in the command-specific structure. Does
> anyone have insight into why it was done that way?
> 
> I further wonder why we have XSM_OTHER: The respective conversion into
> other XSM_* values in xsm/dummy.h could as well move into the callers,
> making intentions more obvious when looking at the actual code.
> 
> --- a/tools/flask/policy/modules/xen.if
> +++ b/tools/flask/policy/modules/xen.if
> @@ -149,7 +149,7 @@ define(`device_model', `
>  	create_channel($2, $1, $2_channel)
>  	allow $1 $2_channel:event create;
>  
> -	allow $1 $2_target:domain shutdown;
> +	allow $1 $2_target:domain { getdomaininfo shutdown };
>  	allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack };
>  	allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel 
> pciroute pcilevel cacheattr send_irq };
>  ')
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -396,14 +396,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>      switch ( op->cmd )
>      {
>      case XEN_DOMCTL_createdomain:
> -    case XEN_DOMCTL_getdomaininfo:
>      case XEN_DOMCTL_test_assign_device:
>      case XEN_DOMCTL_gdbsx_guestmemio:
>          d = NULL;
>          break;
>      default:
>          d = rcu_lock_domain_by_id(op->domain);
> -        if ( d == NULL )
> +        if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
>              return -ESRCH;
>      }
>  
> @@ -817,14 +816,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>  
>      case XEN_DOMCTL_getdomaininfo:
>      {
> -        domid_t dom = op->domain;
> -
> -        rcu_read_lock(&domlist_read_lock);
> +        domid_t dom = DOMID_INVALID;
>  
> -        for_each_domain ( d )
> -            if ( d->domain_id >= dom )
> +        if ( !d )
> +        {
> +            ret = -EINVAL;
> +            if ( op->domain >= DOMID_FIRST_RESERVED )
>                  break;
>  
> +            rcu_read_lock(&domlist_read_lock);
> +
> +            dom = op->domain;
> +            for_each_domain ( d )
> +                if ( d->domain_id >= dom )
> +                    break;
> +        }
> +
>          ret = -ESRCH;
>          if ( d == NULL )
>              goto getdomaininfo_out;
> @@ -839,6 +846,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>          copyback = 1;
>  
>      getdomaininfo_out:
> +        /* When d was non-NULL upon entry, no cleanup is needed. */
> +        if ( dom == DOMID_INVALID )
> +            break;
> +
>          rcu_read_unlock(&domlist_read_lock);
>          d = NULL;
>          break;
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act
>          return 0;
>      case XSM_TARGET:
>          if ( src == target )
> +        {
>              return 0;
> +    case XSM_XS_PRIV:
> +            if ( src->is_xenstore )
> +                return 0;
> +        }
>          /* fall through */
>      case XSM_DM_PRIV:
>          if ( target && src->target == target )
> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act
>          if ( src->is_privileged )
>              return 0;
>          return -EPERM;
> -    case XSM_XS_PRIV:
> -        if ( src->is_xenstore || src->is_privileged )
> -            return 0;
> -        return -EPERM;
>      default:
>          LINKER_BUG_ON(1);
>          return -EPERM;



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

Re: [PATCH v2] domctl: relax getdomaininfo permissions

[Daniel De Graaf]

On 08/11/2016 07:33 AM, Jan Beulich wrote:
>>>> On 05.08.16 at 13:20, <JBeulich@suse.com> wrote:
>
> Daniel,
>
> I've only now realized that I forgot to Cc you on this v2.
>
> Jan
>
>> Qemu needs access to this for the domain it controls, both due to it
>> being used by xc_domain_memory_mapping() (which qemu calls) and the
>> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). Extend
>> permissions to that of any "ordinary" domctl: A domain controlling the
>> targeted domain can invoke this operation for that target domain (which
>> is being achieved by no longer passing NULL to xsm_domctl()).
>>
>> This at once avoids a for_each_domain() loop when the ID of an
>> existing domain gets passed in.
>>
>> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

[...]
> I know there had been an alternative patch suggestion, but that one
> doesn't seem have seen a formal submission so far, so here is my
> original proposal.
>
> I wonder what good the duplication of the returned domain ID does: I'm
> tempted to remove the one in the command-specific structure. Does
> anyone have insight into why it was done that way?
>
> I further wonder why we have XSM_OTHER: The respective conversion into
> other XSM_* values in xsm/dummy.h could as well move into the callers,
> making intentions more obvious when looking at the actual code.

The XSM_* values are not actually present in the XSM hook functions, so
they have to be a static value per function.  Otherwise, the dummy XSM
module won't have enough information to make the same decision as the
inlined dummy.h version does.

An alternate solution would be to add an explicit action parameter to
the hooks that currently use XSM_OTHER, but that mostly just moves the
conversion switch statement around and adds a pointless computation in
the case when the parameter is not used.


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