Re: [PATCH v2 5/6] sparc64: add M3000 Oberon PCIe support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: dma_4u_free_coherent leaves stale IOMMU mappings on non-Oberon platforms
- [Medium] sparc64: missing check for irq_alloc() failure in build_irq() corrupts IRQ 0
- [Low] sparc/pci: missing NULL check for 'reg' property array in DT initializers
--

commit 789a2867a71401d078d82b2a2031ee8cc4266973
Author: Magnus Lindholm <linmag7@gmail.com>

sparc64: add M3000 Oberon PCIe support

This commit adds support for the Fujitsu M3000 Oberon PCIe bridge. It
preserves firmware link setup, handles IOTSB cache line flushes for DVMA
translations, and routes legacy interrupts using the bridge's ten-bit
target ID.

> diff --git a/arch/sparc/kernel/iommu.c b/arch/sparc/kernel/iommu.c
> index 7613ab0ffb89d..9132f0535ce43 100644
> --- a/arch/sparc/kernel/iommu.c
> +++ b/arch/sparc/kernel/iommu.c

[ ... ]

> @@ -253,6 +281,15 @@ static void dma_4u_free_coherent(struct device *dev, size_t size,
>  	npages = IO_PAGE_ALIGN(size) >> IO_PAGE_SHIFT;
>  	iommu = dev->archdata.iommu;
>  
> +	if (iommu->iommu_cache_flush) {
> +		iopte_t *base = iommu->page_table +
> +			((dvma - iommu->tbl.table_map_base) >> IO_PAGE_SHIFT);
> +		unsigned long i;
> +
> +		for (i = 0; i < npages; i++)
> +			iopte_make_dummy(iommu, base + i);
> +		iommu_sync_mapping(iommu, base, npages);
> +	}
>  	iommu_tbl_range_free(&iommu->tbl, dvma, npages, IOMMU_ERROR_CODE);

[Severity: High]
Does this leave stale IOMMU mappings on non-Oberon platforms?

When dma_4u_free_coherent() frees memory, the hardware IOMMU page table
entries (IOPTEs) mapping that memory need to be invalidated before the
physical pages are returned to the kernel allocator.

By guarding the iopte_make_dummy() loop with if (iommu->iommu_cache_flush),
this clears the hardware translation only for Oberon. For other SPARC64
platforms, the hardware IOMMU entries remain valid, potentially allowing
a malfunctioning or malicious device to perform a DMA use-after-free and
corrupt newly re-allocated kernel memory.

> diff --git a/arch/sparc/kernel/irq_64.c b/arch/sparc/kernel/irq_64.c
> index 3f55c69d5f3bc..b0f3b26656d46 100644
> --- a/arch/sparc/kernel/irq_64.c
> +++ b/arch/sparc/kernel/irq_64.c

[ ... ]

> @@ -642,6 +661,17 @@ unsigned int build_irq(int inofixup, unsigned long iclr, unsigned long imap)
>  	return irq;
>  }
>  
> +unsigned int oberon_build_irq(int inofixup, unsigned long iclr,
> +			      unsigned long imap)
> +{
> +	unsigned int irq = build_irq(inofixup, iclr, imap);
> +	struct irq_handler_data *data = irq_get_handler_data(irq);
> +
> +	if (data)
> +		data->oberon = true;
> +	return irq;
> +}

[Severity: Medium]
Is there a missing check for an irq_alloc() failure here?

The newly introduced oberon_build_irq() wrapper calls build_irq().
If irq_alloc() fails during device initialization or under extreme resource
pressure, it returns 0. build_irq() does not check for this 0 return
value and proceeds to use it as a valid IRQ.

This would cause irq_set_chip_and_handler_name() and irq_get_handler_data()
to operate on IRQ 0. Since IRQ 0 is bound to the system timer interrupt
on SPARC64, could this corrupt the system timer and lead to a system hang?

> diff --git a/arch/sparc/kernel/prom_irqtrans.c b/arch/sparc/kernel/prom_irqtrans.c
> index 5752bfd73ac0c..60fd6b0f47e13 100644
> --- a/arch/sparc/kernel/prom_irqtrans.c
> +++ b/arch/sparc/kernel/prom_irqtrans.c

[ ... ]

> @@ -553,6 +556,7 @@ static void __init fire_irq_trans_init(struct device_node *dp)
>  	regs = of_get_property(dp, "reg", NULL);
>  	dp->irq_trans->data = irq_data;
>  
> +	irq_data->oberon = of_device_is_compatible(dp, "pciex108e,80f8");
>  	irq_data->pbm_regs = regs[0].phys_addr;
>  	irq_data->portid = of_getintprop_default(dp, "portid", 0);
>  }

[Severity: Low]
Does this code need a NULL check for the 'reg' property?

of_get_property() can return NULL if the property is missing or malformed.
This would lead to a NULL pointer dereference when accessing
regs[0].phys_addr.

While SPARC OpenBoot PROM typically guarantees the presence of 'reg'
properties for PCI root nodes, making this mostly theoretical in practice,
it appears to violate the API contract. Other sibling drivers enforce
this check. Should we add a NULL check in fire_irq_trans_init() and
pci_fire_pbm_init() to protect the new Oberon PCIe bridge compatibility?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011195729.981243-1-linmag7@gmail.com?part=5