PCI: plda: Fix use-after-free of event IRQs during teardown

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

Pre-existing issues:
- [High] Missing irq_dispose_mapping() before removing IRQ domains causes descriptor leaks and triggers WARN_ON.
- [High] IRQ domains and mappings are permanently leaked if initialization fails in plda_init_interrupts().
- [High] Partial domain creation failure leaks previously allocated domains in plda_pcie_init_irq_domains().
- [High] Unregistering chained interrupt handlers without synchronization leads to Use-After-Free of IRQ domains.
- [Medium] Double registration of IRQ handlers for INTx and MSI events orphans devres actions.
--

--- Patch [1]: [PATCH] PCI: plda: Fix use-after-free of event IRQs during teardown --- commit 9f5cef697b1d53b10380f6d82145d7cc265b3fb6
Author: Ali Tariq <[email protected]>

PCI: plda: Fix use-after-free of event IRQs during teardown

plda_pcie_irq_domain_deinit() removes pcie->event_domain via irq_domain_remove(), but the per-event IRQs mapped from that domain are requested with devm_request_irq() in plda_init_interrupts(). The actual free_irq() for a devm-managed IRQ is deferred by devres until after the calling probe()/remove() function returns.

This means irq_domain_remove() can free the domain's internal data before the deferred free_irq() for IRQs still mapped into it has run. When devres later processes that deferred cleanup, it can end up dereferencing the already-freed domain.

Free each event IRQ explicitly with devm_free_irq() before removing the domain. This triggers the free immediately and removes the IRQ from the devres tracking list, so devres will not attempt to free it a second time later.

[ ... ]

Additional pre-existing issues were found in the same file:

[Severity: High]
This is a pre-existing issue, but does an error in plda_init_interrupts leak the IRQ domains and mappings?

In plda_pcie_host_init:

	ret = plda_init_interrupts(pdev, port, plda_event);
	if (ret)
		goto err_host;
...
err_probe:
	plda_pcie_irq_domain_deinit(port);
err_host:
	if (port->host_ops && port->host_ops->host_deinit)
		port->host_ops->host_deinit(port);

	return ret;

If plda_init_interrupts fails partway through, the goto err_host path bypasses plda_pcie_irq_domain_deinit. Since the domains and mappings allocated up to the failure point are never cleaned up, does this result in a permanent resource leak?

[Severity: High]
This is a pre-existing issue, but does a partial allocation failure during domain creation leak previously created domains?

In plda_pcie_init_irq_domains:

	port->event_domain = irq_domain_create_linear( ... );
	if (!port->event_domain) { ... }

	...

	port->intx_domain = irq_domain_create_linear( ... );
	if (!port->intx_domain) {
		dev_err(dev, "failed to get an INTx IRQ domain\n");
		of_node_put(pcie_intc_node);
		return -ENOMEM;
	}

If the intx_domain allocation fails, the function returns -ENOMEM without destroying the successfully created port->event_domain. Similarly, if plda_allocate_msi_domains fails later, neither domain is freed. Should this error path clean up the already-allocated domains?

[Severity: Medium]
This is a pre-existing issue, but does overwriting the flow handlers orphan the previously registered actions?

In plda_init_interrupts:

	for_each_set_bit(i, &port->events_bitmap, port->num_events) {
		event_irq = irq_create_mapping(port->event_domain, i);
		...
			ret = devm_request_irq(dev, event_irq,
					       plda_event_handler,
					       0, NULL, port);
		...
	}

	port->intx_irq = irq_create_mapping(port->event_domain,
					    event->intx_event);
	...
	/* Plug the INTx chained handler */
	irq_set_chained_handler_and_data(port->intx_irq, plda_handle_intx, port);

The events_bitmap typically includes intx_event and msi_event. The loop registers a threaded irqaction via devm_request_irq for these events. Immediately after, irq_set_chained_handler_and_data overwrites the flow handler for the exact same virtual IRQs.

By replacing the generic flow handler with a custom chained handler, the original action registered by devm_request_irq is permanently bypassed and orphaned. Does this needlessly consume memory and leave inactive actions until teardown?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1