Re: [PATCH 1/2] PCI: shpchp: Release MSI and MMIO resources on init failure

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Introduction of legacy PCI MSI API pci_disable_msi() instead of modern IRQ vector APIs.

Pre-existing issues:
- [Critical] Potential Use-After-Free due to asynchronous timer teardown using timer_delete().
- [Critical] Potential Use-After-Free due to asynchronous delayed work cancellation in cleanup_slots().
--

commit defb7c8ebb6a52714afc91c46d2b3a41539a49d0
Author: Myeonghun Pak <mhun512@gmail.com>

PCI: shpchp: Release MSI and MMIO resources on init failure

If request_irq() fails after enabling MSI, shpc_init() unmaps the
registers but leaves MSI enabled and the MMIO region reserved. Disable
MSI before unmapping and release the reserved region on the failure path.

> diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
> index 183bf43510a10..4492c7d2bc934 100644
> --- a/drivers/pci/hotplug/shpchp_hpc.c
> +++ b/drivers/pci/hotplug/shpchp_hpc.c

[ ... ]

> @@ -1041,8 +1040,11 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
>  	return 0;
>  
>  	/* We end up here for the many possible ways to fail this API.  */
> -abort_iounmap:
> +abort_disable_msi:
> +	pci_disable_msi(pdev);

[Severity: Low]
Does this code introduce a new use of the deprecated legacy MSI API?

The PCI subsystem guidelines state that legacy APIs like pci_disable_msi()
must not be used in new code. Could this driver be modernized to use the
generic IRQ vector allocation interfaces pci_alloc_irq_vectors() and
pci_free_irq_vectors() instead?

>  	iounmap(ctrl->creg);
> +abort_release_region:
> +	release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
>  abort:
>  	return rc;
>  }

While reviewing the teardown paths, I noticed a couple of pre-existing
issues:

drivers/pci/hotplug/shpchp_hpc.c:shpchp_release_ctlr() {
	...
	if (shpchp_poll_mode)
		timer_delete(&ctrl->poll_timer);

[Severity: Critical]
This isn't a bug introduced by this patch, but can this asynchronous timer
teardown cause a Use-After-Free?

During module removal, shpc_remove() calls shpchp_release_ctlr(), which
invokes timer_delete(&ctrl->poll_timer). This does not wait for a
concurrently executing timer handler (int_poll_timeout()) to finish on
another CPU.

Immediately after shpchp_release_ctlr() returns, shpc_remove() frees the
ctrl structure via kfree(ctrl).

If the timer handler is still executing, could it access the freed ctrl
structure and attempt to re-arm the timer with start_int_poll_timer(),
resulting in memory corruption?

Should timer_delete_sync() be used here to ensure teardown symmetry?

	else {
		free_irq(ctrl->pci_dev->irq, ctrl);
		pci_disable_msi(ctrl->pci_dev);
	}
	...
}


drivers/pci/hotplug/shpchp_core.c:cleanup_slots() {
	...
	list_for_each_entry_safe(slot, next, &ctrl->slot_list, slot_list) {
		list_del(&slot->slot_list);
		cancel_delayed_work(&slot->work);
		destroy_workqueue(slot->wq);
		pci_hp_deregister(&slot->hotplug_slot);
		kfree(slot);

[Severity: Critical]
This isn't a bug introduced by this patch, but can this asynchronous
delayed work cancellation lead to a Use-After-Free?

In cleanup_slots(), cancel_delayed_work() does not wait if the delayed
work timer handler (delayed_work_timer_fn) is currently executing.

While destroy_workqueue() waits for already-queued works to execute, it
does not wait for the timer handler that queues them.

If the timer handler runs concurrently, could it access the freed slot
structure to read dwork->cpu and dwork->wq, resulting in a Use-After-Free
on the slot and an invalid enqueue to a destroyed workqueue?

Would cancel_delayed_work_sync() be required here to ensure the timer
handler has completely finished?

	}
}

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