linux-kernel - Re: [PATCH] genirq/msi: fix populating multiple interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <858dec65-6980-184f-8851-59c45d13f9e3@arm.com>
Date:   Wed, 6 Sep 2017 09:52:43 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     John Keeping <john@...anate.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] genirq/msi: fix populating multiple interrupts

Hi John,

On 05/09/17 18:12, John Keeping wrote:
> Use the correct variable to set up each interrupt in turn rather than
> configuring the first interrupt "nvec" times.

Thanks for addressing this. I think this bug deserves a slightly better
write-up. How about something like:

<quote>
On allocating the interrupts routed via to a wire-to-MSI bridge, we
iterate over the MSI descriptors to build the hierarchy, but fail to use
the descriptor interrupt number, and instead use the base number,
generating the wrong IRQ domain mappings.

The fix is to use the MSI descriptor interrupt number when setting up
the interrupt instead of the base interrupt for the allocation range.

The only saving grace is that although the MSI descriptors are allocated
in bulk, the wired interrupts are only allocated one by one (so
desc->irq == virq) and the bug goes unnoticed.
</quote>

> Signed-off-by: John Keeping <john@...anate.com>
> ---
>  kernel/irq/msi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 48eadf416c24..3fa4bd59f569 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -315,11 +315,12 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
>  
>  		ops->set_desc(arg, desc);
>  		/* Assumes the domain mutex is held! */
> -		ret = irq_domain_alloc_irqs_hierarchy(domain, virq, 1, arg);
> +		ret = irq_domain_alloc_irqs_hierarchy(domain, desc->irq, 1,
> +						      arg);
>  		if (ret)
>  			break;
>  
> -		irq_set_msi_desc_off(virq, 0, desc);
> +		irq_set_msi_desc_off(desc->irq, 0, desc);
>  	}
>  
>  	if (ret) {
> 

Fixes: 2145ac9310b60 ("genirq/msi: Add msi_domain_populate_irqs")
Cc: stable@...r.kernel.org #v4.5+
Reviewed-by: Marc Zyngier <marc.zyngier@....com>

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...