linux-kernel - Re: [PATCH] irqchip: gic-v2m: Handle multiple MSI base IRQ mis-alignment

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 for Android: free password hash cracker in your pocket

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

Message-ID: <8634a39hg3.wl-maz@kernel.org>
Date: Thu, 07 Aug 2025 18:20:44 +0100
From: Marc Zyngier <maz@...nel.org>
To: Christian Bruel <christian.bruel@...s.st.com>
Cc: <tglx@...utronix.de>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>,
	<fabrice.gasnier@...s.st.com>,
	<mani@...nel.org>
Subject: Re: [PATCH] irqchip: gic-v2m: Handle multiple MSI base IRQ mis-alignment

On Thu, 07 Aug 2025 12:47:58 +0100,
Christian Bruel <christian.bruel@...s.st.com> wrote:
> 
> The PCI Local Bus Specification (section 6.8.3.4 in Rev 3) allows modifying
> the low-order bits of the MSI Message Data to encode the interrupt number.
> However, the base SPI used for allocation may not be aligned to the
> requested number of irqs.
> 
> For instance, on STM32MP25 with an initial MSI_TYPER base SPI of 0x16A,
> allocating a multiple MSI of size 8 with the first two slots reserved, the
> offset returned is 8, resulting in an MSI DATA base of 0x172.
> This causes the endpoint device to send a wrong interrupt number:
> 
> 1st MSI = 0x172 | 0x0 = 0x172
> 2nd MSI = 0x172 | 0x1 = 0x173
> 3rd MSI = 0x172 | 0x2 = 0x172 wrongly triggers the 1st MSI
> 
> The alignment offset can be computed in the gicv2m driver:
> replacing bitmap_find_free_region() with bitmap_find_next_zero_area_off()
> to accommodate the required alignment.

Well, that's only telling half of the story. We get there because the
SPI range has been allocated in a pretty dumb manner (the starting SPI
is not a multiple of 32). Pretty damning if you're considering PCI.

> 
> Without this fix, the workaround is to force the alignment in the DT
> within the MSI range (if 32 MSIs are mapped from 362 to 393):
> arm,msi-base-spi = <368>
> arm,msi-num-spis = <26>
> with the effect of reducing the number of available MSIs.

This doesn't really belong here. But the example is pretty wrong
anyway, because what you need for PCI is the low 5 bits to be clear,
so that you can allocate 32 MSIs. So your range would start at 384,
and... oh wait...

> 
> Change-Id: I316a580755cd1b1684929d2540295f4a45f0532d

Neither does this.

> Signed-off-by: Christian Bruel <christian.bruel@...s.st.com>
> ---
>  drivers/irqchip/irq-gic-v2m.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
> index 24ef5af569fe..21a14d15e7a9 100644
> --- a/drivers/irqchip/irq-gic-v2m.c
> +++ b/drivers/irqchip/irq-gic-v2m.c
> @@ -153,14 +153,18 @@ static int gicv2m_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  {
>  	msi_alloc_info_t *info = args;
>  	struct v2m_data *v2m = NULL, *tmp;
> -	int hwirq, offset, i, err = 0;
> +	int hwirq, i, err = 0;
> +	unsigned long align_mask = (1 << get_count_order(nr_irqs)) - 1;

Use BIT().

> +	unsigned long align_off, offset;
>  
>  	spin_lock(&v2m_lock);
>  	list_for_each_entry(tmp, &v2m_nodes, entry) {
> -		offset = bitmap_find_free_region(tmp->bm, tmp->nr_spis,
> -						 get_count_order(nr_irqs));
> -		if (offset >= 0) {
> +		align_off = tmp->spi_start & info->desc->pci.msi_attrib.multiple;

Err, no. MSIs are not just PCI only, and there is no reason why you
should go and rummage into these data structures. That's none of an
irqchip's business. The correct way to go about this is to consider
nr_irqs, because that's what you are trying to align for.

But I really don't get how you compute that offset. 'multiple' is the
number of bits required to encode the number of vectors. How does this
work?

> +		offset = bitmap_find_next_zero_area_off(tmp->bm, tmp->nr_spis, 0,
> +							nr_irqs, align_mask, align_off);
> +		if (offset < tmp->nr_spis) {
>  			v2m = tmp;
> +			bitmap_set(v2m->bm, offset, nr_irqs);
>  			break;
>  		}
>  	}

	M.

-- 
Without deviation from the norm, progress is not possible.