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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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