Re: [PATCH 1/3] PCI: apple: Fix ECAM config window use-after-free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] PCI: apple: incomplete IRQ and domain teardown in
  pci_host_common_remove()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] PCI: apple: MSI irq_domain leak and use-after-free in
  apple_pcie_probe() [1]
--

commit 6595d3b06ad4c1310264ede81b930782370102fe
Author: Hongyan Xu <getshell@seu.edu.cn>

PCI: apple: Fix ECAM config window use-after-free

This commit wires pci_host_common_remove() as the platform driver remove
callback so the PCI root bus is removed before devres frees the ECAM
configuration window.

> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index c2cffc0659f44..a1e8a3a0ef1dc 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -886,6 +886,7 @@ MODULE_DEVICE_TABLE(of, apple_pcie_of_match);
>  
>  static struct platform_driver apple_pcie_driver = {
>  	.probe	= apple_pcie_probe,

[Severity: Critical]
This problem wasn't introduced by this patch, but does apple_pcie_probe()
leak the MSI parent irq_domain [1] if pci_host_common_init() fails?

apple_msi_init() creates an unmanaged IRQ domain via
msi_create_parent_irq_domain() with host_data set to the devm-allocated
struct apple_pcie before calling pci_host_common_init():

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);
}

If pci_host_common_init() fails or defers (for example when
apple_pcie_setup_port() defers on GPIO or reset acquisition), devres frees
bridge and pcie->bitmap while the MSI irq_domain stays on irq_domain_list
with a dangling host_data pointer, causing a use-after-free on subsequent
domain lookups or probe retries.

> +	.remove	= pci_host_common_remove,

[Severity: High]
Does wiring pci_host_common_remove() alone leave the unmanaged IRQ
domains, port interrupts, and chained handlers active after rmmod?

During probe, apple_msi_init() registers an unmanaged MSI parent domain
via msi_create_parent_irq_domain(), and for each port
apple_pcie_setup_port() takes an OF node reference via of_node_get(),
creates a linear IRQ domain via irq_domain_create_linear(), sets
apple_port_irq_handler() via irq_set_chained_handler_and_data(), and
requests link up/down virqs via request_irq().

pci_host_common_remove() only removes the PCI root bus.  When rmmod
runs, devres frees struct apple_pcie and struct apple_pcie_port and unmaps
MMIO, and free_module() unmaps the module text and rodata while those IRQ
domains and handlers remain registered.  Reading /proc/interrupts or
receiving a port interrupt then faults on unmapped module memory and
freed port state.  Should this driver either use builtin_platform_driver()
(matching .suppress_bind_attrs = true) or provide a remove callback that
masks port interrupts and tears down the requested IRQs, chained handlers,
IRQ domains, and OF node references?

[1] https://sashiko.dev/#/bug/linux-93248aea-1b69-486f-9e9f-a90f7de2b348

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108105640.875-1-getshell@seu.edu.cn?part=1