From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] x86/IO-APIC: don't create pIRQ mapping from
 masked RTE
Date: Fri, 21 Aug 2015 17:06:55 +0100
Message-ID: <55D74C9F.1010005@citrix.com>
References: <55D7005C020000780009C57C@prv-mh.provo.novell.com>
	<55D73C80.8050908@citrix.com>
	<55D7616B020000780009C6F5@prv-mh.provo.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <prvs=668b0e4c2=Andrew.Cooper3@citrix.com>)
	id 1ZSoqs-0005KZ-9L
	for xen-devel@lists.xenproject.org; Fri, 21 Aug 2015 16:07:34 +0000
In-Reply-To: <55D7616B020000780009C6F5@prv-mh.provo.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>, Keir Fraser <keir@xen.org>, Wei Liu <wei.liu2@citrix.com>
List-Id: xen-devel@lists.xenproject.org

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