Re: [PATCH] PCI: mediatek: Find INTx controller by property

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] PCIe IRQ domains are leaked and retain dangling pointers to devm-freed ports on various probe error paths, leading to Use-After-Free.
--

--- Patch [1]: [PATCH] PCI: mediatek: Find INTx controller by property ---
commit 8e34608d98c97419f127cc199be1c423e10e10a5
Author: Zhengping Zhang <aquapinn@qq.com>

PCI: mediatek: Find INTx controller by property

All existing DTS users already name the INTx child node
"interrupt-controller". Use of_get_child_by_name() to find it instead
of assuming it is the first child node.

> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 8b57c3d75b942..7b54f3dc6cf87 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -572,9 +572,9 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
>  	int ret;
>  
>  	/* Setup INTx */
> -	pcie_intc_node = of_get_next_child(node, NULL);
> +	pcie_intc_node = of_get_child_by_name(node, "interrupt-controller");
>  	if (!pcie_intc_node) {
> -		dev_err(dev, "no PCIe Intc node found\n");
> +		dev_err(dev, "no PCIe INTx interrupt-controller node found\n");
>  		return -ENODEV;
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but does this code leak the IRQ
domains on probe error paths, leading to a Use-After-Free?

The port structure is allocated with devm_kzalloc(), but port->irq_domain is
created here via irq_domain_create_linear() which is not devres-managed. The
port structure is set as the domain's host_data.

If probe fails later in mtk_pcie_setup_irq() because platform_get_irq() fails:

drivers/pci/controller/pcie-mediatek.c:mtk_pcie_setup_irq() {
    err = mtk_pcie_init_irq_domain(port, node);
    ...
    if (port->irq < 0)
        return port->irq;
}

The code directly returns an error without removing the IRQ domains. 
This also happens if parsing a subsequent port fails in mtk_pcie_setup():

drivers/pci/controller/pcie-mediatek.c:mtk_pcie_setup() {
    ...
        err = mtk_pcie_parse_port(pcie, child, slot);
        if (err)
            return err;
}

Or if pci_host_probe() fails in mtk_pcie_probe():

drivers/pci/controller/pcie-mediatek.c:mtk_pcie_probe() {
    ...
    err = pci_host_probe(host);
    if (err)
        goto put_resources;
    ...
put_resources:
    if (!list_empty(&pcie->ports))
        mtk_pcie_put_resources(pcie);
    return err;
}

In this path, mtk_pcie_put_resources() frees the ports, but
mtk_pcie_irq_teardown() is never called.

When probe fails, devres frees the port, but the globally registered IRQ
domain remains. A subsequent probe attempt or another device translating
an IRQ from the same fwnode could find the leaked domain and dereference
its freed host_data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_5E7D6E7FB25B4CD732F08547C11F5DA24808@qq.com?part=1