[PATCH v2] domctl: tighten XEN_DOMCTL_*_permission

[PATCH v2] domctl: tighten XEN_DOMCTL_*_permission

[Jan Beulich]

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

With proper permission (and, for the I/O port case, wrap-around) checks
added (note that for the I/O port case a count of zero is now being
disallowed, in line with I/O memory handling):

XEN_DOMCTL_irq_permission:
XEN_DOMCTL_ioport_permission:

 Of both IRQs and I/O ports there is only a reasonably small amount, so
 there's no excess resource consumption involved here. Additionally
 they both have a specialized XSM hook associated.

XEN_DOMCTL_iomem_permission:

 While this also has a specialized XSM hook associated (just like
 XEN_DOMCTL_{irq,ioport}_permission), it's not clear whether it's
 reasonable to expect XSM to restrict the number of ranges associated
 with a domain via this hook (which is the main resource consumption
 item here).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Fix off-by-one in XEN_DOMCTL_ioport_permission bounds checking.

--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -72,9 +72,7 @@ __HYPERVISOR_domctl (xen/include/public/
  * XEN_DOMCTL_getvcpucontext
  * XEN_DOMCTL_max_vcpus
  * XEN_DOMCTL_scheduler_op
- * XEN_DOMCTL_irq_permission
  * XEN_DOMCTL_iomem_permission
- * XEN_DOMCTL_ioport_permission
  * XEN_DOMCTL_gethvmcontext
  * XEN_DOMCTL_sethvmcontext
  * XEN_DOMCTL_set_address_size
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -46,6 +46,8 @@ static int gdbsx_guest_mem_io(
     return (iop->remain ? -EFAULT : 0);
 }
 
+#define MAX_IOPORTS 0x10000
+
 long arch_do_domctl(
     struct xen_domctl *domctl, struct domain *d,
     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
@@ -72,13 +74,11 @@ long arch_do_domctl(
         unsigned int np = domctl->u.ioport_permission.nr_ports;
         int allow = domctl->u.ioport_permission.allow_access;
 
-        ret = -EINVAL;
-        if ( (fp + np) > 65536 )
-            break;
-
-        if ( np == 0 )
-            ret = 0;
-        else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
+        if ( (fp + np) <= fp || (fp + np) > MAX_IOPORTS )
+            ret = -EINVAL;
+        else if ( !ioports_access_permitted(current->domain,
+                                            fp, fp + np - 1) ||
+                  xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
             ret = -EPERM;
         else if ( allow )
             ret = ioports_permit_access(d, fp, fp + np - 1);
@@ -719,7 +719,6 @@ long arch_do_domctl(
 
     case XEN_DOMCTL_ioport_mapping:
     {
-#define MAX_IOPORTS    0x10000
         struct hvm_iommu *hd;
         unsigned int fgp = domctl->u.ioport_mapping.first_gport;
         unsigned int fmp = domctl->u.ioport_mapping.first_mport;
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
         if ( pirq >= d->nr_pirqs )
             ret = -EINVAL;
-        else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
+        else if ( !pirq_access_permitted(current->domain, pirq) ||
+                  xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
             ret = -EPERM;
         else if ( allow )
             ret = pirq_permit_access(d, pirq);
@@ -809,7 +810,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
             break;
 
-        if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
+        if ( !iomem_access_permitted(current->domain,
+                                     mfn, mfn + nr_mfns - 1) ||
+             xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
             ret = -EPERM;
         else if ( allow )
             ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);




[-- Attachment #2: domctl-permit-access.patch --]
[-- Type: text/plain, Size: 3795 bytes --]

domctl: tighten XEN_DOMCTL_*_permission

With proper permission (and, for the I/O port case, wrap-around) checks
added (note that for the I/O port case a count of zero is now being
disallowed, in line with I/O memory handling):

XEN_DOMCTL_irq_permission:
XEN_DOMCTL_ioport_permission:

 Of both IRQs and I/O ports there is only a reasonably small amount, so
 there's no excess resource consumption involved here. Additionally
 they both have a specialized XSM hook associated.

XEN_DOMCTL_iomem_permission:

 While this also has a specialized XSM hook associated (just like
 XEN_DOMCTL_{irq,ioport}_permission), it's not clear whether it's
 reasonable to expect XSM to restrict the number of ranges associated
 with a domain via this hook (which is the main resource consumption
 item here).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Fix off-by-one in XEN_DOMCTL_ioport_permission bounds checking.

--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -72,9 +72,7 @@ __HYPERVISOR_domctl (xen/include/public/
  * XEN_DOMCTL_getvcpucontext
  * XEN_DOMCTL_max_vcpus
  * XEN_DOMCTL_scheduler_op
- * XEN_DOMCTL_irq_permission
  * XEN_DOMCTL_iomem_permission
- * XEN_DOMCTL_ioport_permission
  * XEN_DOMCTL_gethvmcontext
  * XEN_DOMCTL_sethvmcontext
  * XEN_DOMCTL_set_address_size
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -46,6 +46,8 @@ static int gdbsx_guest_mem_io(
     return (iop->remain ? -EFAULT : 0);
 }
 
+#define MAX_IOPORTS 0x10000
+
 long arch_do_domctl(
     struct xen_domctl *domctl, struct domain *d,
     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
@@ -72,13 +74,11 @@ long arch_do_domctl(
         unsigned int np = domctl->u.ioport_permission.nr_ports;
         int allow = domctl->u.ioport_permission.allow_access;
 
-        ret = -EINVAL;
-        if ( (fp + np) > 65536 )
-            break;
-
-        if ( np == 0 )
-            ret = 0;
-        else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
+        if ( (fp + np) <= fp || (fp + np) > MAX_IOPORTS )
+            ret = -EINVAL;
+        else if ( !ioports_access_permitted(current->domain,
+                                            fp, fp + np - 1) ||
+                  xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
             ret = -EPERM;
         else if ( allow )
             ret = ioports_permit_access(d, fp, fp + np - 1);
@@ -719,7 +719,6 @@ long arch_do_domctl(
 
     case XEN_DOMCTL_ioport_mapping:
     {
-#define MAX_IOPORTS    0x10000
         struct hvm_iommu *hd;
         unsigned int fgp = domctl->u.ioport_mapping.first_gport;
         unsigned int fmp = domctl->u.ioport_mapping.first_mport;
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
         if ( pirq >= d->nr_pirqs )
             ret = -EINVAL;
-        else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
+        else if ( !pirq_access_permitted(current->domain, pirq) ||
+                  xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
             ret = -EPERM;
         else if ( allow )
             ret = pirq_permit_access(d, pirq);
@@ -809,7 +810,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
             break;
 
-        if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
+        if ( !iomem_access_permitted(current->domain,
+                                     mfn, mfn + nr_mfns - 1) ||
+             xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
             ret = -EPERM;
         else if ( allow )
             ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);

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

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

Re: [PATCH v2] domctl: tighten XEN_DOMCTL_*_permission

[Tim Deegan]

At 14:08 +0100 on 06 May (1399381725), Jan Beulich wrote:
> With proper permission (and, for the I/O port case, wrap-around) checks
> added (note that for the I/O port case a count of zero is now being
> disallowed, in line with I/O memory handling):
> 
> XEN_DOMCTL_irq_permission:
> XEN_DOMCTL_ioport_permission:
> 
>  Of both IRQs and I/O ports there is only a reasonably small amount, so
>  there's no excess resource consumption involved here. Additionally
>  they both have a specialized XSM hook associated.
> 
> XEN_DOMCTL_iomem_permission:
> 
>  While this also has a specialized XSM hook associated (just like
>  XEN_DOMCTL_{irq,ioport}_permission), it's not clear whether it's
>  reasonable to expect XSM to restrict the number of ranges associated
>  with a domain via this hook (which is the main resource consumption
>  item here).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH v2] domctl: tighten XEN_DOMCTL_*_permission

[Andrii Tseglytskyi]

Hi,

I see possible issue with this patch. Can someone clarify - did I get
everything correctly?

On Tue, May 6, 2014 at 4:08 PM, Jan Beulich <JBeulich@suse.com> wrote:
> With proper permission (and, for the I/O port case, wrap-around) checks
> added (note that for the I/O port case a count of zero is now being
> disallowed, in line with I/O memory handling):
>
> XEN_DOMCTL_irq_permission:
> XEN_DOMCTL_ioport_permission:
>
>  Of both IRQs and I/O ports there is only a reasonably small amount, so
>  there's no excess resource consumption involved here. Additionally
>  they both have a specialized XSM hook associated.
>
> XEN_DOMCTL_iomem_permission:
>
>  While this also has a specialized XSM hook associated (just like
>  XEN_DOMCTL_{irq,ioport}_permission), it's not clear whether it's
>  reasonable to expect XSM to restrict the number of ranges associated
>  with a domain via this hook (which is the main resource consumption
>  item here).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Fix off-by-one in XEN_DOMCTL_ioport_permission bounds checking.
>
> --- a/docs/misc/xsm-flask.txt
> +++ b/docs/misc/xsm-flask.txt
> @@ -72,9 +72,7 @@ __HYPERVISOR_domctl (xen/include/public/
>   * XEN_DOMCTL_getvcpucontext
>   * XEN_DOMCTL_max_vcpus
>   * XEN_DOMCTL_scheduler_op
> - * XEN_DOMCTL_irq_permission
>   * XEN_DOMCTL_iomem_permission
> - * XEN_DOMCTL_ioport_permission
>   * XEN_DOMCTL_gethvmcontext
>   * XEN_DOMCTL_sethvmcontext
>   * XEN_DOMCTL_set_address_size
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -46,6 +46,8 @@ static int gdbsx_guest_mem_io(
>      return (iop->remain ? -EFAULT : 0);
>  }
>
> +#define MAX_IOPORTS 0x10000
> +
>  long arch_do_domctl(
>      struct xen_domctl *domctl, struct domain *d,
>      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> @@ -72,13 +74,11 @@ long arch_do_domctl(
>          unsigned int np = domctl->u.ioport_permission.nr_ports;
>          int allow = domctl->u.ioport_permission.allow_access;
>
> -        ret = -EINVAL;
> -        if ( (fp + np) > 65536 )
> -            break;
> -
> -        if ( np == 0 )
> -            ret = 0;
> -        else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
> +        if ( (fp + np) <= fp || (fp + np) > MAX_IOPORTS )
> +            ret = -EINVAL;
> +        else if ( !ioports_access_permitted(current->domain,
> +                                            fp, fp + np - 1) ||
> +                  xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
>              ret = -EPERM;
>          else if ( allow )
>              ret = ioports_permit_access(d, fp, fp + np - 1);
> @@ -719,7 +719,6 @@ long arch_do_domctl(
>
>      case XEN_DOMCTL_ioport_mapping:
>      {
> -#define MAX_IOPORTS    0x10000
>          struct hvm_iommu *hd;
>          unsigned int fgp = domctl->u.ioport_mapping.first_gport;
>          unsigned int fmp = domctl->u.ioport_mapping.first_mport;
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>
>          if ( pirq >= d->nr_pirqs )
>              ret = -EINVAL;
> -        else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
> +        else if ( !pirq_access_permitted(current->domain, pirq) ||

pirq_access_permitted() checks a range. Range can be added only with
pirq_permit_access() function call. The only place where
pirq_permit_access() is called - is following
 *else if* branch. But it will be never called -
pirq_access_permitted() will return 0 if range does not exist. As
result - it is impossible to add irq, even if XSM allows this.
The same is true for iomem_access_permitted() function call.


> +                  xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
>              ret = -EPERM;
>          else if ( allow )
>              ret = pirq_permit_access(d, pirq);
> @@ -809,7 +810,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>          if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
>              break;
>
> -        if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
> +        if ( !iomem_access_permitted(current->domain,
> +                                     mfn, mfn + nr_mfns - 1) ||
> +             xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
>              ret = -EPERM;
>          else if ( allow )
>              ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>



-- 

Andrii Tseglytskyi | Embedded Dev
GlobalLogic
www.globallogic.com

Re: [PATCH v2] domctl: tighten XEN_DOMCTL_*_permission

[Andrew Cooper]

On 15/08/14 11:00, Andrii Tseglytskyi wrote:
> Hi,
>
> I see possible issue with this patch. Can someone clarify - did I get
> everything correctly?
>
> On Tue, May 6, 2014 at 4:08 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> @@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>
>>          if ( pirq >= d->nr_pirqs )
>>              ret = -EINVAL;
>> -        else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
>> +        else if ( !pirq_access_permitted(current->domain, pirq) ||
> pirq_access_permitted() checks a range. Range can be added only with
> pirq_permit_access() function call. The only place where
> pirq_permit_access() is called - is following
>  *else if* branch. But it will be never called -
> pirq_access_permitted() will return 0 if range does not exist. As
> result - it is impossible to add irq, even if XSM allows this.
> The same is true for iomem_access_permitted() function call.

I questioned the same issue when this patch went in.

The argument is that, even with XSM, a domain may only permit access to
pirqs for which it also has permissions.

This prevents a buggy domain builder accidentally conferring pirq access
for a dom0 resource, without dom0 first having conferred access to the
domain builder.

~Andrew

Re: [PATCH v2] domctl: tighten XEN_DOMCTL_*_permission

[Andrii Tseglytskyi]

Hi Andrew,

Thank you for your comment.

On Fri, Aug 15, 2014 at 1:36 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 15/08/14 11:00, Andrii Tseglytskyi wrote:
>> Hi,
>>
>> I see possible issue with this patch. Can someone clarify - did I get
>> everything correctly?
>>
>> On Tue, May 6, 2014 at 4:08 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>> @@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>>
>>>          if ( pirq >= d->nr_pirqs )
>>>              ret = -EINVAL;
>>> -        else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
>>> +        else if ( !pirq_access_permitted(current->domain, pirq) ||
>> pirq_access_permitted() checks a range. Range can be added only with
>> pirq_permit_access() function call. The only place where
>> pirq_permit_access() is called - is following
>>  *else if* branch. But it will be never called -
>> pirq_access_permitted() will return 0 if range does not exist. As
>> result - it is impossible to add irq, even if XSM allows this.
>> The same is true for iomem_access_permitted() function call.
>
> I questioned the same issue when this patch went in.
>
> The argument is that, even with XSM, a domain may only permit access to
> pirqs for which it also has permissions.
>
> This prevents a buggy domain builder accidentally conferring pirq access
> for a dom0 resource, without dom0 first having conferred access to the
> domain builder.
>

Okay. This sounds reasonable.
One more question - I see that pirq_access_permitted() calls with
current->domain pointer, which points to dom0.
So, if resource belongs to dom0 - it must not belong to any other
domain. But in current implementation pirq_access_permitted() returns
0 if resource is not found.
In other words - resource does not belong to dom0, but -EPERM error
will be returned anyway.

Regards,
Andrii

> ~Andrew



-- 

Andrii Tseglytskyi | Embedded Dev
GlobalLogic
www.globallogic.com

Re: [PATCH v2] domctl: tighten XEN_DOMCTL_*_permission

[Andrew Cooper]

On 15/08/14 12:02, Andrii Tseglytskyi wrote:
> Hi Andrew,
>
> Thank you for your comment.
>
> On Fri, Aug 15, 2014 at 1:36 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 15/08/14 11:00, Andrii Tseglytskyi wrote:
>>> Hi,
>>>
>>> I see possible issue with this patch. Can someone clarify - did I get
>>> everything correctly?
>>>
>>> On Tue, May 6, 2014 at 4:08 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> @@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>>>
>>>>          if ( pirq >= d->nr_pirqs )
>>>>              ret = -EINVAL;
>>>> -        else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
>>>> +        else if ( !pirq_access_permitted(current->domain, pirq) ||
>>> pirq_access_permitted() checks a range. Range can be added only with
>>> pirq_permit_access() function call. The only place where
>>> pirq_permit_access() is called - is following
>>>  *else if* branch. But it will be never called -
>>> pirq_access_permitted() will return 0 if range does not exist. As
>>> result - it is impossible to add irq, even if XSM allows this.
>>> The same is true for iomem_access_permitted() function call.
>> I questioned the same issue when this patch went in.
>>
>> The argument is that, even with XSM, a domain may only permit access to
>> pirqs for which it also has permissions.
>>
>> This prevents a buggy domain builder accidentally conferring pirq access
>> for a dom0 resource, without dom0 first having conferred access to the
>> domain builder.
>>
> Okay. This sounds reasonable.
> One more question - I see that pirq_access_permitted() calls with
> current->domain pointer, which points to dom0.

In a traditional case, yes.  In an XSM case with semi-privileged
domain-builder domains, current could easily point to a domain other
than dom0.

> So, if resource belongs to dom0 - it must not belong to any other
> domain.

Why so?  Resources are not exclusive to a single domain.  PCIPassthough
is a prime example where the device strictly still belongs to dom0, but
pciback in dom0 ensures it is not being used by dom0 while it is being
used by another domain.

>  But in current implementation pirq_access_permitted() returns
> 0 if resource is not found.

Correct, and that is the point.

If the current domain does not have permissions to use a certain pirq,
it does not have permission to alter the permission of another domain,
with respect to this pirq.  Note that by permitting/denying access to a
target domain, the current domain does not loose its existing permissions.

> In other words - resource does not belong to dom0, but -EPERM error
> will be returned anyway.

The resource may not strictly belong to dom0, but dom0 still retains
permissions to use it, and permissions to revoke access in the future if
it chooses.

~Andrew

Re: [PATCH v2] domctl: tighten XEN_DOMCTL_*_permission

[Andrii Tseglytskyi]

Hi Andrew,

>>  But in current implementation pirq_access_permitted() returns
>> 0 if resource is not found.
>
> Correct, and that is the point.
>
> If the current domain does not have permissions to use a certain pirq,
> it does not have permission to alter the permission of another domain,
> with respect to this pirq.  Note that by permitting/denying access to a
> target domain, the current domain does not loose its existing permissions.
>

Thank you. This is the answer for my question. Now got it.

Regards,
Andrii


-- 

Andrii Tseglytskyi | Embedded Dev
GlobalLogic
www.globallogic.com