Re: [PATCH v3 2/3] PCI: dw-rockchip: Make the INTx irq setup devm-managed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PCI: dw-rockchip: INTx IRQ domain created too late for child devices
- [High] PCI: dw-rockchip: missing .hwirq_max causes INTx IRQ mappings to fail
- [High] PCI: dw-rockchip: missing synchronize_irq() in chained IRQ release causes UAF
--

commit 94fa1be31fa024fc300334850bba499a495b5954
Author: Shawn Lin <shawn.lin@rock-chips.com>

PCI: dw-rockchip: Make the INTx irq setup devm-managed

The irq domain and the chained handler set up by the previous commit
are resources owned by the device, but nothing releases them if probing
ever fails after they are set up, and they are not tied to the device
lifetime in any other way either.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 59d9a1b095df6..561c296c770eb 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
[ ... ]
> @@ -187,9 +188,16 @@ static const struct irq_domain_ops intx_domain_ops = {
>  	.map = rockchip_pcie_intx_map,
>  };
>  
> -static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
> +static void rockchip_pcie_intx_chained_release(void *data)
> +{
> +	struct rockchip_pcie *rockchip = data;
> +
> +	irq_set_chained_handler_and_data(rockchip->intx_irq, NULL, NULL);

[Severity: High]
Does this release function need a call to synchronize_irq() to prevent a
use-after-free?

Calling irq_set_chained_handler_and_data() with NULL masks the IRQ and removes
the handler, but it does not wait for any currently executing chained handlers
on other CPUs to finish.

Since this is a devres cleanup action, the IRQ domain and the rockchip
structure (which was allocated with devm_kzalloc) will be destroyed and freed
shortly after this returns. If rockchip_pcie_intx_handler() is running
concurrently during probe failure, it could dereference the freed rockchip
structure or its destroyed irq_domain.

> +}
> +
> +static int rockchip_pcie_init_irq_domain(struct device *dev,
> +					 struct rockchip_pcie *rockchip)
>  {
[ ... ]
> -	rockchip->irq_domain = irq_domain_create_linear(of_fwnode_handle(intc), PCI_NUM_INTX,
> -							&intx_domain_ops, rockchip);
> +	rockchip->irq_domain = devm_irq_domain_instantiate(dev,
> +			&(struct irq_domain_info){
> +				.fwnode = of_fwnode_handle(intc),
> +				.size = PCI_NUM_INTX,
> +				.ops = &intx_domain_ops,
> +				.host_data = rockchip,
> +			});

[Severity: High]
Does this compound literal need to initialize .hwirq_max?

By omitting .hwirq_max, it defaults to 0. The core IRQ subsystem
(__irq_domain_create) assigns domain->hwirq_max = info->hwirq_max.

During interrupt mapping, irq_domain_associate_locked() validates mappings
with WARN(hwirq >= domain->hwirq_max). Because hwirq_max is 0, any valid
hardware IRQ (e.g., 0 to 3) will trigger this warning and cause the mapping
to abort with -EINVAL, disabling all INTx interrupts.

[ ... ]
> @@ -730,18 +743,27 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
>  	 * which is also re-run by .reset_root_port(), so that the INTx irq
>  	 * domain is only created once, at probe time.
>  	 */
> -	irq = of_irq_get_byname(dev->of_node, "legacy");
> -	if (irq < 0)
> -		return irq;
> +	rockchip->intx_irq = of_irq_get_byname(dev->of_node, "legacy");
> +	if (rockchip->intx_irq < 0)
> +		return rockchip->intx_irq;
>  
> -	ret = rockchip_pcie_init_irq_domain(rockchip);
> +	ret = rockchip_pcie_init_irq_domain(dev, rockchip);

[Severity: High]
Is this IRQ domain instantiated too late for child devices?

Earlier in rockchip_pcie_configure_rc(), dw_pcie_host_init() is called, which
invokes pci_host_probe() to enumerate the PCIe bus and probe downstream device
drivers.

Because the legacy INTx domain is not yet instantiated when those child
devices probe, pci_assign_irq() will fail to find the legacy domain, breaking
INTx interrupt mapping for downstream devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1790044622-164744-1-git-send-email-shawn.lin@rock-chips.com?part=2