Re: [PATCH v2 1/2] PCI: dw-rockchip: Move the INTx irq setup to probe and make it devm-managed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Probe failure after dw_pcie_host_init() skips required dw_pcie_host_deinit() cleanup, causing a severe use-after-free and resource leak.
- [High] PCI host enumerated before INTx IRQ domain is created, causing downstream legacy interrupts to unconditionally fail.

Pre-existing issues:
- [High] Early SYS IRQ firing causes NULL pointer dereference in rockchip_pcie_rc_sys_irq_thread.
--

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

PCI: dw-rockchip: Move the INTx irq setup to probe and make it devm-managed

This commit moves the INTx IRQ domain creation and chained handler setup
into the probe function, managing their lifetimes with devres. This prevents
IRQ domain leaks and use-after-free issues during root port resets.

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

[ ... ]

> @@ -739,6 +739,33 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
>  		return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but does the current driver initialization
order risk a NULL pointer dereference in rockchip_pcie_rc_sys_irq_thread()?

The SYS IRQ is requested with devm_request_threaded_irq() earlier in
rockchip_pcie_configure_rc(), before dw_pcie_host_init() is called.
If the IRQ fires immediately (for example, if the bootloader leaves a
pending link-down reset status), rockchip_pcie_rc_sys_irq_thread() will
iterate over pp->bridge->bus. However, pp->bridge is not allocated until
dw_pcie_host_init() runs, which would cause a crash.

>  
> +	/*
> +	 * 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.
> +	 */
> +	rockchip->intx_irq = of_irq_get_byname(dev->of_node, "legacy");
> +	if (rockchip->intx_irq < 0)
> +		return rockchip->intx_irq;

[Severity: High]
If of_irq_get_byname() fails here, the function returns directly. Since
dw_pcie_host_init() has already successfully executed and registered the
PCI host bridge, does this path need to call dw_pcie_host_deinit() before
returning? 

Without explicit cleanup, devres will free the rockchip_pcie structure on
probe failure, while the PCI subsystem might still retain active references
to the host controller. Can this cause a use-after-free?

> +
> +	ret = rockchip_pcie_init_irq_domain(dev, rockchip);

[Severity: High]
Since the INTx IRQ domain is now instantiated after dw_pcie_host_init()
has completed, will this cause legacy interrupts for downstream devices to
fail unconditionally?

The dw_pcie_host_init() call enumerates the PCI bus and probes downstream
devices. If those devices attempt to request legacy INTx interrupts during
their probe, the INTx IRQ domain will not exist yet.

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

[Severity: High]
Similarly to the above error path, returning here skips tearing down the
enumerated PCI host via dw_pcie_host_deinit(), which leaves dangling
references to the host structure.

> +
> +	irq_set_chained_handler_and_data(rockchip->intx_irq,
> +					 rockchip_pcie_intx_handler, rockchip);
> +
> +	/*
> +	 * Uninstall the chained handler on probe failure, so that it can
> +	 * never run against the devm-freed rockchip structure.
> +	 */
> +	ret = devm_add_action_or_reset(dev, rockchip_pcie_intx_chained_release,
> +				       rockchip);
> +	if (ret)
> +		return ret;

[Severity: High]
This error path also skips dw_pcie_host_deinit(). Should all of these new
error returns be updated to properly unregister the PCI host?

> +
>  	/* 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/1788484709-139123-1-git-send-email-shawn.lin@rock-chips.com?part=1