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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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