Re: [PATCH] x86/IO-APIC: don't create pIRQ mapping from masked RTE

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

On 21/08/15 16:35, Jan Beulich wrote:
>>>> On 21.08.15 at 16:58, <andrew.cooper3@citrix.com> wrote:
>> On 21/08/15 09:41, Jan Beulich wrote:
>>> In the course of this I also found that the respective message isn't
>>> really useful without also printing the pre-existing mapping. And I
>>> noticed that map_domain_pirq() allowed IRQ0 to get through, despite us
>>> never allowing and domain to control that interrupt.
>> s/and/a/ ? (I can't quite parse the original statement)
> Ouch - "a" was meant.
>
>> Also, doesn't irq_access_permitted() catch the irq0 case?
> It should, yes, but ...
>
>>> --- a/xen/arch/x86/irq.c
>>> +++ b/xen/arch/x86/irq.c
>>> @@ -1906,7 +1906,7 @@ int map_domain_pirq(
>>>      if ( !irq_access_permitted(current->domain, irq))
>>>          return -EPERM;
>> I would be tempted to put a comment here stating that irq0 is definitely
>> a xen-reserved irq.  Otherwise, it is odd to have the mismatch between
>> pirq and irq.
> Such a "mismatch" was already there (albeit I wouldn't call it a
> mismatch, since from an abstract pov the two number spaces are
> entirely distinct), specifically ...
>
>>> -    if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >= nr_irqs )
>>> +    if ( pirq < 0 || pirq >= d->nr_pirqs || irq <= 0 || irq >= nr_irqs )
>>>      {
>>>          dprintk(XENLOG_G_ERR, "dom%d: invalid pirq %d or irq %d\n",
>>>                  d->domain_id, pirq, irq);
>>> @@ -1919,8 +1919,9 @@ int map_domain_pirq(
>>>      if ( (old_irq > 0 && (old_irq != irq) ) ||
>>>           (old_pirq && (old_pirq != pirq)) )
> ... here.

Right, but irq0 is a valid irq which makes it suspicious to drop it at
the validity check.

I am not fussed too much with bikeshedding the issue.

~Andrew