Re: [PATCH v3 1/3] PCI: dw-rockchip: Move the INTx irq setup to probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pcie: missing dw_pcie_host_deinit() on error paths causes UAF
- [High] pcie: late INTx IRQ domain registration breaks downstream INTx interrupts
--

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

PCI: dw-rockchip: Move the INTx irq setup to probe

The commit moves INTx IRQ domain setup out of the host ops .init() callback
and into rockchip_pcie_configure_rc() to avoid leaking the domain upon
controller reset. It performs the IRQ initialization immediately after
dw_pcie_host_init() completes.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index af26a07c0c9e5..59d9a1b095df6 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

[ ... ]

> @@ -739,6 +725,24 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
>  		return ret;
>  	}
>  
> +	/*
> +	 * This is done here instead of in the host ops .init() callback,
> +	 * 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");

[Severity: High]
Does moving the INTx IRQ domain creation here cause downstream devices to lose
their INTx interrupts?

Since rockchip_pcie_configure_rc() calls dw_pcie_host_init() right before this,
which internally invokes pci_host_probe() to enumerate the PCIe bus, downstream
devices will be probed before this new code is reached.  When those devices
attempt to map their legacy INTx interrupts via the device tree, they won't
find the IRQ domain since it hasn't been created yet, which seems like it would
cause the mapping to silently fail.

This architectural issue appears to remain unaddressed at the end of the patch
series.

> +	if (irq < 0)
> +		return irq;
> +
> +	ret = rockchip_pcie_init_irq_domain(rockchip);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to init irq domain\n");
> +		return ret;
> +	}

[Severity: High]
Can these error paths cause a use-after-free by leaving the PCI root bus
actively registered?

Since dw_pcie_host_init() successfully allocated memory, registered the PCI
host bridge, and exposed the bus to the system, returning an error here
without calling dw_pcie_host_deinit() will leave the bus active.  Because
the driver uses devres for struct rockchip_pcie, any subsequent interaction
with the bus could trigger a use-after-free on the freed rockchip structure.

This issue does not appear to be resolved by the follow-up patches in this
series.

> +
> +	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
> +					 rockchip);
> +
>  	/* unmask hot reset/link-down reset */
>  	val = FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
>  	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);

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