Re: [PATCH] PCI: apple: use fwnode_irq_get() for port IRQ lookup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing teardown for partially initialized ports in `apple_pcie_init()` leaves dangling chained IRQ handlers and leaked IRQ domains, leading to a Use-After-Free on probe error.
- [High] MSI parent IRQ domain is leaked on probe failure, leading to Use-After-Free on re-probe.
--

--- Patch [1]: [PATCH] PCI: apple: use fwnode_irq_get() for port IRQ lookup ---
commit eab8bdc8d6046daad9b1ca1595a1c3a4a1b10540
Author: Rosen Penev <rosenp@gmail.com>

PCI: apple: use fwnode_irq_get() for port IRQ lookup

Replace irq_of_parse_and_map() with fwnode_irq_get() on the PCIe
controller device's fwnode. Propagate the error instead of checking
for zero, allowing negative errors like -EPROBE_DEFER to be handled.

> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index c2cffc0659f44..535acf274b786 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -400,14 +400,13 @@ static int apple_pcie_port_setup_irq(struct apple_pcie_port *port)
>  {
>  	struct fwnode_handle *fwnode = &port->np->fwnode;
>  	struct apple_pcie *pcie = port->pcie;
> -	unsigned int irq;
> +	int irq;
>  	u32 val = 0;
>  
>  	/* FIXME: consider moving each interrupt under each port */
> -	irq = irq_of_parse_and_map(to_of_node(dev_fwnode(port->pcie->dev)),
> -				   port->idx);
> -	if (!irq)
> -		return -ENXIO;
> +	irq = fwnode_irq_get(dev_fwnode(pcie->dev), port->idx);
> +	if (irq < 0)
> +		return irq;

[Severity: High]
This isn't a bug introduced by this patch, but returning the error here
(which can now be -EPROBE_DEFER) exposes a missing teardown path in
apple_pcie_init().

When apple_pcie_port_setup_irq() returns an error, apple_pcie_setup_port()
and then apple_pcie_init() abort without tearing down previously initialized
ports:

drivers/pci/controller/pcie-apple.c:apple_pcie_init() {
    ...
    for_each_available_child_of_node_scoped(dev->of_node, of_port) {
        ret = apple_pcie_setup_port(pcie, of_port);
        if (ret) {
            dev_err(dev, "Port %pOF setup fail: %d\n", of_port, ret);
            return ret;
        }
    }
    ...
}

Because the chained handlers (irq_set_chained_handler_and_data) and IRQ
domains (irq_domain_create_linear) set up for previous ports are not managed
by devres, they remain active. 

Devres will free the port structures, so if an interrupt fires or a re-probe
occurs, won't apple_port_irq_handler() dereference a freed port pointer?

[Severity: High]
This is also a pre-existing issue, but propagating an error like
-EPROBE_DEFER here makes a leak in apple_pcie_probe() much more reachable.

If this error propagates up and causes pci_host_common_init() to fail in
apple_pcie_probe(), the MSI parent IRQ domain created earlier by
apple_msi_init() is not removed:

drivers/pci/controller/pcie-apple.c:apple_pcie_probe() {
    ...
    ret = apple_msi_init(pcie);
    if (ret)
        return ret;
    ...
    return pci_host_common_init(pdev, bridge, &apple_pcie_cfg_ecam_ops);
}

The domain created via msi_create_parent_irq_domain() is not devres-managed.
Devres will free the pcie structure on error, leaving the leaked domain's
host_data pointing to freed memory. 

On re-probe, IRQ lookups will match the leaked domain, and
apple_msi_domain_alloc() will dereference the freed pcie pointer. 

Should there be manual cleanup for the MSI domain when pci_host_common_init()
fails?

>  
>  	port->domain = irq_domain_create_linear(fwnode, 32,
>  						&apple_port_irq_domain_ops,

[ ... ]

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