[PATCH] domctl: fix IRQ permission granting/revocation

[PATCH] domctl: fix IRQ permission granting/revocation

[Jan Beulich]

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

Commit 545607eb3c ("x86: fix various issues with handling guest IRQs")
wasn't really consistent in one respect: The granting of access to an
IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both
domains. In fact it is wrong to assume that a translation is already/
still in place at the time access is being granted/revoked.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     case XEN_DOMCTL_irq_permission:
     {
-        unsigned int pirq = op->u.irq_permission.pirq;
+        unsigned int pirq = op->u.irq_permission.pirq, irq;
         int allow = op->u.irq_permission.allow_access;
 
         if ( pirq >= d->nr_pirqs )
             ret = -EINVAL;
-        else if ( !pirq_access_permitted(current->domain, pirq) ||
+        else if ( !(irq = 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);
+            ret = irq_permit_access(d, irq);
         else
-            ret = pirq_deny_access(d, pirq);
+            ret = irq_deny_access(d, irq);
     }
     break;
 
--- a/xen/include/xen/iocap.h
+++ b/xen/include/xen/iocap.h
@@ -28,22 +28,11 @@
 #define irq_access_permitted(d, i)                      \
     rangeset_contains_singleton((d)->irq_caps, i)
 
-#define pirq_permit_access(d, i) ({                     \
-    struct domain *d__ = (d);                           \
-    int i__ = domain_pirq_to_irq(d__, i);               \
-    i__ > 0 ? rangeset_add_singleton(d__->irq_caps, i__)\
-            : -EINVAL;                                  \
-})
-#define pirq_deny_access(d, i) ({                       \
-    struct domain *d__ = (d);                           \
-    int i__ = domain_pirq_to_irq(d__, i);               \
-    i__ > 0 ? rangeset_remove_singleton(d__->irq_caps, i__)\
-            : -EINVAL;                                  \
-})
 #define pirq_access_permitted(d, i) ({                  \
     struct domain *d__ = (d);                           \
-    rangeset_contains_singleton(d__->irq_caps,          \
-                                domain_pirq_to_irq(d__, i));\
+    int irq__ = domain_pirq_to_irq(d__, i);             \
+    irq__ > 0 && irq_access_permitted(d__, irq__)       \
+    ? irq__ : 0;                                        \
 })
 
 #endif /* __XEN_IOCAP_H__ */




[-- Attachment #2: domctl-irq-permission.patch --]
[-- Type: text/plain, Size: 2630 bytes --]

domctl: fix IRQ permission granting/revocation

Commit 545607eb3c ("x86: fix various issues with handling guest IRQs")
wasn't really consistent in one respect: The granting of access to an
IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both
domains. In fact it is wrong to assume that a translation is already/
still in place at the time access is being granted/revoked.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
     case XEN_DOMCTL_irq_permission:
     {
-        unsigned int pirq = op->u.irq_permission.pirq;
+        unsigned int pirq = op->u.irq_permission.pirq, irq;
         int allow = op->u.irq_permission.allow_access;
 
         if ( pirq >= d->nr_pirqs )
             ret = -EINVAL;
-        else if ( !pirq_access_permitted(current->domain, pirq) ||
+        else if ( !(irq = 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);
+            ret = irq_permit_access(d, irq);
         else
-            ret = pirq_deny_access(d, pirq);
+            ret = irq_deny_access(d, irq);
     }
     break;
 
--- a/xen/include/xen/iocap.h
+++ b/xen/include/xen/iocap.h
@@ -28,22 +28,11 @@
 #define irq_access_permitted(d, i)                      \
     rangeset_contains_singleton((d)->irq_caps, i)
 
-#define pirq_permit_access(d, i) ({                     \
-    struct domain *d__ = (d);                           \
-    int i__ = domain_pirq_to_irq(d__, i);               \
-    i__ > 0 ? rangeset_add_singleton(d__->irq_caps, i__)\
-            : -EINVAL;                                  \
-})
-#define pirq_deny_access(d, i) ({                       \
-    struct domain *d__ = (d);                           \
-    int i__ = domain_pirq_to_irq(d__, i);               \
-    i__ > 0 ? rangeset_remove_singleton(d__->irq_caps, i__)\
-            : -EINVAL;                                  \
-})
 #define pirq_access_permitted(d, i) ({                  \
     struct domain *d__ = (d);                           \
-    rangeset_contains_singleton(d__->irq_caps,          \
-                                domain_pirq_to_irq(d__, i));\
+    int irq__ = domain_pirq_to_irq(d__, i);             \
+    irq__ > 0 && irq_access_permitted(d__, irq__)       \
+    ? irq__ : 0;                                        \
 })
 
 #endif /* __XEN_IOCAP_H__ */

[-- 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] domctl: fix IRQ permission granting/revocation

[Ian Campbell]

On Wed, 2014-12-10 at 08:07 +0000, Jan Beulich wrote:
> Commit 545607eb3c ("x86: fix various issues with handling guest IRQs")
> wasn't really consistent in one respect: The granting of access to an
> IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both
> domains. In fact it is wrong to assume that a translation is already/
> still in place at the time access is being granted/revoked.

Specifically you need to do the translation using the mapping of the
domain doing the granting, not the domain being granted too, correct?

It takes a little bit of thought to figure out which domain to check
here, it would be worth a sentence or two explaining why this is the
right one.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>  
>      case XEN_DOMCTL_irq_permission:
>      {
> -        unsigned int pirq = op->u.irq_permission.pirq;
> +        unsigned int pirq = op->u.irq_permission.pirq, irq;
>          int allow = op->u.irq_permission.allow_access;
>  
>          if ( pirq >= d->nr_pirqs )
>              ret = -EINVAL;
> -        else if ( !pirq_access_permitted(current->domain, pirq) ||
> +        else if ( !(irq = pirq_access_permitted(current->domain, pirq)) ||

I find hiding an assignment inside the second condition in a chain of
if's to be rather obfuscated. Doing an assignment in a standalone if
statement is one thing, this is going to far IMHO.

Also, you range check pirq against d->nr_pirqs but then translate it
against current->domain, is that correct?

Ian.

Re: [PATCH] domctl: fix IRQ permission granting/revocation

[Jan Beulich]

>>> On 10.12.14 at 10:53, <Ian.Campbell@eu.citrix.com> wrote:
> On Wed, 2014-12-10 at 08:07 +0000, Jan Beulich wrote:
>> Commit 545607eb3c ("x86: fix various issues with handling guest IRQs")
>> wasn't really consistent in one respect: The granting of access to an
>> IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both
>> domains. In fact it is wrong to assume that a translation is already/
>> still in place at the time access is being granted/revoked.
> 
> Specifically you need to do the translation using the mapping of the
> domain doing the granting, not the domain being granted too, correct?
> 
> It takes a little bit of thought to figure out which domain to check
> here, it would be worth a sentence or two explaining why this is the
> right one.

Would

"What is wanted is to translate the incoming pIRQ to an IRQ for
 the invoking domain (as the pIRQ is the only notion the invoking
 domain has of the IRQ), and grant the subject domain access to
 the resulting IRQ."

make this more clear without being purely redundant with the code?

Jan

Re: [PATCH] domctl: fix IRQ permission granting/revocation

[Ian Campbell]

On Wed, 2014-12-10 at 10:00 +0000, Jan Beulich wrote:
> >>> On 10.12.14 at 10:53, <Ian.Campbell@eu.citrix.com> wrote:
> > On Wed, 2014-12-10 at 08:07 +0000, Jan Beulich wrote:
> >> Commit 545607eb3c ("x86: fix various issues with handling guest IRQs")
> >> wasn't really consistent in one respect: The granting of access to an
> >> IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both
> >> domains. In fact it is wrong to assume that a translation is already/
> >> still in place at the time access is being granted/revoked.
> > 
> > Specifically you need to do the translation using the mapping of the
> > domain doing the granting, not the domain being granted too, correct?
> > 
> > It takes a little bit of thought to figure out which domain to check
> > here, it would be worth a sentence or two explaining why this is the
> > right one.
> 
> Would
> 
> "What is wanted is to translate the incoming pIRQ to an IRQ for
>  the invoking domain (as the pIRQ is the only notion the invoking
>  domain has of the IRQ), and grant the subject domain access to
>  the resulting IRQ."
> 
> make this more clear without being purely redundant with the code?

Yes, thanks.

Ian.

Re: [PATCH] domctl: fix IRQ permission granting/revocation

[Julien Grall]

Hi Jan,

On 10/12/2014 08:07, Jan Beulich wrote:
> Commit 545607eb3c ("x86: fix various issues with handling guest IRQs")
> wasn't really consistent in one respect: The granting of access to an
> IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both
> domains. In fact it is wrong to assume that a translation is already/
> still in place at the time access is being granted/revoked.

With the change to the interface, some part of libxl may misuse 
xc_domain_irq_permission. For instance in tools/libxl/libxl_create.c:

1178         ret = irq >= 0 ? xc_physdev_map_pirq(CTX->xch, domid, irq, 
&irq)

We get the PIRQ of domain domid in irq.

1179                        : -EOVERFLOW;
1180         if (!ret)
1181             ret = xc_domain_irq_permission(CTX->xch, domid, irq,
1)

Here, the PIRQ of the current domain should be passed. Fortunately, for 
this specific case, the PIRQs are the same. But this is confusing.

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>
>       case XEN_DOMCTL_irq_permission:
>       {
> -        unsigned int pirq = op->u.irq_permission.pirq;
> +        unsigned int pirq = op->u.irq_permission.pirq, irq;
>           int allow = op->u.irq_permission.allow_access;
>
>           if ( pirq >= d->nr_pirqs )
>               ret = -EINVAL;
> -        else if ( !pirq_access_permitted(current->domain, pirq) ||
> +        else if ( !(irq = pirq_access_permitted(current->domain, pirq)) ||
>                     xsm_irq_permission(XSM_HOOK, d, pirq, allow) )

As the pirq may not be the same in domain d, the XSM permission is wrong 
here.

In anycase, it looks weird to use pirq here because the guy who defines 
the policy may not know the PIRQ value.

Regards,

-- 
Julien Grall

Re: [PATCH] domctl: fix IRQ permission granting/revocation

[Jan Beulich]

>>> On 10.12.14 at 11:19, <julien.grall@linaro.org> wrote:
> Hi Jan,
> 
> On 10/12/2014 08:07, Jan Beulich wrote:
>> Commit 545607eb3c ("x86: fix various issues with handling guest IRQs")
>> wasn't really consistent in one respect: The granting of access to an
>> IRQ shouldn't assume the pIRQ->IRQ translation to be the same in both
>> domains. In fact it is wrong to assume that a translation is already/
>> still in place at the time access is being granted/revoked.
> 
> With the change to the interface, some part of libxl may misuse 
> xc_domain_irq_permission. For instance in tools/libxl/libxl_create.c:
> 
> 1178         ret = irq >= 0 ? xc_physdev_map_pirq(CTX->xch, domid, irq, 
> &irq)
> 
> We get the PIRQ of domain domid in irq.
> 
> 1179                        : -EOVERFLOW;
> 1180         if (!ret)
> 1181             ret = xc_domain_irq_permission(CTX->xch, domid, irq,
> 1)
> 
> Here, the PIRQ of the current domain should be passed. Fortunately, for 
> this specific case, the PIRQs are the same. But this is confusing.

Agreed, but I'd leave it to the tools maintainers to clean this up.

Jan

Re: [PATCH] domctl: fix IRQ permission granting/revocation

[Jan Beulich]

>>> On 10.12.14 at 10:53, <Ian.Campbell@eu.citrix.com> wrote:
> On Wed, 2014-12-10 at 08:07 +0000, Jan Beulich wrote:
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>  
>>      case XEN_DOMCTL_irq_permission:
>>      {
>> -        unsigned int pirq = op->u.irq_permission.pirq;
>> +        unsigned int pirq = op->u.irq_permission.pirq, irq;
>>          int allow = op->u.irq_permission.allow_access;
>>  
>>          if ( pirq >= d->nr_pirqs )
>>              ret = -EINVAL;
>> -        else if ( !pirq_access_permitted(current->domain, pirq) ||
>> +        else if ( !(irq = pirq_access_permitted(current->domain, pirq)) ||
> 
> I find hiding an assignment inside the second condition in a chain of
> if's to be rather obfuscated. Doing an assignment in a standalone if
> statement is one thing, this is going to far IMHO.

Changed. My main intention was to avoid having to add another
"break;".

> Also, you range check pirq against d->nr_pirqs but then translate it
> against current->domain, is that correct?

No, it's not. As much as xsm_irq_permission(XSM_HOOK, d, pirq, allow)
is bogus, yet it's not clear to me whether switching that to use the Xen
IRQ number is okay without any other changes. Daniel?

Jan

Re: [PATCH] domctl: fix IRQ permission granting/revocation

[Daniel De Graaf]

On 12/11/2014 06:44 AM, Jan Beulich wrote:
>>>> On 10.12.14 at 10:53, <Ian.Campbell@eu.citrix.com> wrote:
>> On Wed, 2014-12-10 at 08:07 +0000, Jan Beulich wrote:
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -981,18 +981,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>>>
>>>       case XEN_DOMCTL_irq_permission:
>>>       {
>>> -        unsigned int pirq = op->u.irq_permission.pirq;
>>> +        unsigned int pirq = op->u.irq_permission.pirq, irq;
>>>           int allow = op->u.irq_permission.allow_access;
>>>
>>>           if ( pirq >= d->nr_pirqs )
>>>               ret = -EINVAL;
>>> -        else if ( !pirq_access_permitted(current->domain, pirq) ||
>>> +        else if ( !(irq = pirq_access_permitted(current->domain, pirq)) ||
>>
>> I find hiding an assignment inside the second condition in a chain of
>> if's to be rather obfuscated. Doing an assignment in a standalone if
>> statement is one thing, this is going to far IMHO.
>
> Changed. My main intention was to avoid having to add another
> "break;".
>
>> Also, you range check pirq against d->nr_pirqs but then translate it
>> against current->domain, is that correct?
>
> No, it's not. As much as xsm_irq_permission(XSM_HOOK, d, pirq, allow)
> is bogus, yet it's not clear to me whether switching that to use the Xen
> IRQ number is okay without any other changes. Daniel?
>
> Jan

At the moment, this XSM hook does not inspect the PIRQ argument, so it
is safe to change it to use the Xen IRQ number.  The XSM hooks currently
only inspect the IRQ at mapping time; with this change (and the prior
changes that fixed up the IRQ permissions rangesets), it may make sense
to either add a check here or move the check in order to catch the error
earlier.

-- 
Daniel De Graaf
National Security Agency