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

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

[Jan Beulich]

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.

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Also fix initial range check to use current->domain, adjust code
    structure, and extend description (all requested by Ian). Along
    the lines of the first mentioned change, also pass the Xen IRQ
    number to the XSM hook (confirmed okay by Daniel).
Note that I would hope for this to make unnecessary Stefano's proposed
tools side change
http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00160.html.

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -982,18 +982,21 @@ 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 )
+        if ( pirq >= current->domain->nr_pirqs )
+        {
             ret = -EINVAL;
-        else if ( !pirq_access_permitted(current->domain, pirq) ||
-                  xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
+            break;
+        }
+        irq = pirq_access_permitted(current->domain, pirq);
+        if ( !irq || xsm_irq_permission(XSM_HOOK, d, irq, 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__ */

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

[Andrew Cooper]

On 12/12/14 10:24, 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.
>
> 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.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Should domain_pirq_to_irq() be using 0 as its default invalid value,
rather than -1?  irq 0 is a real irq and could plausibly be wanted to be
passed through to a guest.

~Andrew

> ---
> v2: Also fix initial range check to use current->domain, adjust code
>     structure, and extend description (all requested by Ian). Along
>     the lines of the first mentioned change, also pass the Xen IRQ
>     number to the XSM hook (confirmed okay by Daniel).
> Note that I would hope for this to make unnecessary Stefano's proposed
> tools side change
> http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00160.html.
>
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -982,18 +982,21 @@ 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 )
> +        if ( pirq >= current->domain->nr_pirqs )
> +        {
>              ret = -EINVAL;
> -        else if ( !pirq_access_permitted(current->domain, pirq) ||
> -                  xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
> +            break;
> +        }
> +        irq = pirq_access_permitted(current->domain, pirq);
> +        if ( !irq || xsm_irq_permission(XSM_HOOK, d, irq, 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__ */
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 12.12.14 at 11:49, <andrew.cooper3@citrix.com> wrote:
> On 12/12/14 10:24, 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.
>>
>> 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.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Should domain_pirq_to_irq() be using 0 as its default invalid value,
> rather than -1?  irq 0 is a real irq and could plausibly be wanted to be
> passed through to a guest.

Not on x86. If another architecture would ever need this, I think
we'd need to audit all current users of domain_pirq_to_irq() before
doing such a change.

Jan

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

[Ian Campbell]

On Fri, 2014-12-12 at 11:03 +0000, Jan Beulich wrote:
> >>> On 12.12.14 at 11:49, <andrew.cooper3@citrix.com> wrote:
> > On 12/12/14 10:24, 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.
> >>
> >> 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.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Should domain_pirq_to_irq() be using 0 as its default invalid value,
> > rather than -1?  irq 0 is a real irq and could plausibly be wanted to be
> > passed through to a guest.
> 
> Not on x86. If another architecture would ever need this, I think
> we'd need to audit all current users of domain_pirq_to_irq() before
> doing such a change.

FWIW on ARM (at least the versions we support, i.e. with the generic IRQ
controller) IRQ0 is an SGI (what x86 would call an IPI). It seems
unlikely we'd want to pass one of those through...

Ian.

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

[Ian Campbell]

On Fri, 2014-12-12 at 10:24 +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.
> 
> 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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
> v2: Also fix initial range check to use current->domain, adjust code
>     structure, and extend description (all requested by Ian). Along
>     the lines of the first mentioned change, also pass the Xen IRQ
>     number to the XSM hook (confirmed okay by Daniel).
> Note that I would hope for this to make unnecessary Stefano's proposed
> tools side change
> http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00160.html.
> 
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -982,18 +982,21 @@ 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 )
> +        if ( pirq >= current->domain->nr_pirqs )
> +        {
>              ret = -EINVAL;
> -        else if ( !pirq_access_permitted(current->domain, pirq) ||
> -                  xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
> +            break;
> +        }
> +        irq = pirq_access_permitted(current->domain, pirq);
> +        if ( !irq || xsm_irq_permission(XSM_HOOK, d, irq, 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__ */
> 
> 
>