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

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

[Myeonghun Pak]

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. Reuse the region cleanup for an ioremap() failure.
pci_disable_msi() also handles the INTx fallback where MSI was not enabled.

These omissions are already present in the initial Git import.

This issue was identified during our ongoing static-analysis research
while reviewing kernel code.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
Assisted-by: OpenAI:GPT-5.6
---
Validated with an ARM64 W=1 object build and strict checkpatch.
No hardware runtime, IRQ-failure injection or hotplug testing was done.

diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
index 183bf43..4492c7d 100644
--- a/drivers/pci/hotplug/shpchp_hpc.c
+++ b/drivers/pci/hotplug/shpchp_hpc.c
@@ -943,9 +943,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
 	if (!ctrl->creg) {
 		ctrl_err(ctrl, "Cannot remap MMIO region %lx @ %lx\n",
 			 ctrl->mmio_size, ctrl->mmio_base);
-		release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
 		rc = -1;
-		goto abort;
+		goto abort_release_region;
 	}
 	ctrl_dbg(ctrl, "ctrl->creg %p\n", ctrl->creg);
 
@@ -1008,7 +1007,7 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
 		if (rc) {
 			ctrl_err(ctrl, "Can't get irq %d for the hotplug controller\n",
 				 ctrl->pci_dev->irq);
-			goto abort_iounmap;
+			goto abort_disable_msi;
 		}
 	}
 	ctrl_dbg(ctrl, "HPC at %s irq=%x\n", pci_name(pdev), pdev->irq);
@@ -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);
 	iounmap(ctrl->creg);
+abort_release_region:
+	release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
 abort:
 	return rc;
 }

[PATCH 2/2] PCI: shpchp: Balance PCI device enable on teardown

[Myeonghun Pak]

shpc_init() enables the PCI device, but neither initialization failure
after that point nor shpchp_release_ctlr() balances the enable.

Disable the device after releasing the controller resources. Route
reservation failures directly to the device cleanup, while failures
before pci_enable_device() succeeds continue to skip it.

The missing disable is already present in the initial Git import.

This issue was identified during our ongoing static-analysis research
while reviewing kernel code.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
Assisted-by: OpenAI:GPT-5.6
---
This patch depends on patch 1's error-unwind labels.
Validated with an ARM64 W=1 object build and strict checkpatch.
No hardware runtime, IRQ-failure injection or hotplug testing was done.

diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
index 4492c7d..e00e4fb 100644
--- a/drivers/pci/hotplug/shpchp_hpc.c
+++ b/drivers/pci/hotplug/shpchp_hpc.c
@@ -572,6 +572,7 @@ void shpchp_release_ctlr(struct controller *ctrl)
 
 	iounmap(ctrl->creg);
 	release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
+	pci_disable_device(ctrl->pci_dev);
 }
 
 int shpchp_power_on_slot(struct slot *slot)
@@ -936,7 +937,7 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)
 	if (!request_mem_region(ctrl->mmio_base, ctrl->mmio_size, MY_NAME)) {
 		ctrl_err(ctrl, "Cannot reserve MMIO region\n");
 		rc = -1;
-		goto abort;
+		goto abort_disable_device;
 	}
 
 	ctrl->creg = ioremap(ctrl->mmio_base, ctrl->mmio_size);
@@ -1045,6 +1046,8 @@ abort_disable_msi:
 	iounmap(ctrl->creg);
 abort_release_region:
 	release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
+abort_disable_device:
+	pci_disable_device(pdev);
 abort:
 	return rc;
 }

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

[sashiko-bot]

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

Re: [PATCH 2/2] PCI: shpchp: Balance PCI device enable on teardown

[sashiko-bot]

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

New issues:
- [High] Balancing `pci_enable_device()` on a PCI bridge breaks active downstream devices.

Pre-existing issues:
- [High] Unsafe timer deletion using timer_delete() instead of timer_delete_sync() for a self-rearming timer leads to potential Use-After-Free.
- [High] Stopping background interrupt/timer tasks AFTER freeing slot data structures leads to list corruption and Use-After-Free.
- [High] Potential NULL pointer dereference in shpc_isr() because hardware interrupts are unmasked before the slot list is initialized.
--

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

PCI: shpchp: Balance PCI device enable on teardown

This patch attempts to balance pci_enable_device() by adding calls to
pci_disable_device() during driver teardown and initialization failure. It
routes reservation failures directly to device cleanup, while skipping it for
failures prior to enablement.

> diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c
> index 4492c7d2bc934..e00e4fb3b522c 100644
> --- a/drivers/pci/hotplug/shpchp_hpc.c
> +++ b/drivers/pci/hotplug/shpchp_hpc.c
> @@ -572,6 +572,7 @@ void shpchp_release_ctlr(struct controller *ctrl)

[Severity: High]
This is a pre-existing issue, but does freeing the slot data structures before
stopping the background tasks lead to a use-after-free or list corruption?

Earlier in shpchp_release_ctlr(), cleanup_slots() locklessly removes and frees
all slot structures before the interrupt handler or timer is stopped. If
shpc_isr() responds to a concurrent interrupt or timer, it calls
shpchp_find_slot(), which iterates over the freed elements in ctrl->slot_list:

drivers/pci/hotplug/shpchp.h:shpchp_find_slot() {
    ...
	list_for_each_entry(slot, &ctrl->slot_list, slot_list) {
		if (slot->device == device)
			return slot;
	}
    ...
}

Should we stop the interrupts and timers before cleaning up the slots?

[Severity: High]
This is a pre-existing issue, but can using timer_delete() instead of
timer_delete_sync() in shpchp_release_ctlr() introduce a potential
use-after-free?

If int_poll_timeout() is running concurrently on another CPU, it unconditionally
re-arms itself using start_int_poll_timer(). Since the driver teardown code
continues and frees the ctrl structure, could the re-armed timer access freed
memory when it fires?

>  	iounmap(ctrl->creg);
>  	release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
> +	pci_disable_device(ctrl->pci_dev);

[Severity: High]
Is it safe to disable the PCI-to-PCI bridge abruptly here?

Due to a known PCI core design flaw, pci_enable_bridge() does not increment
the bridge's enable_cnt when child devices enable it if it is already enabled.
Calling pci_disable_device() here drops the count to 0 and clears the
PCI_COMMAND_MASTER bit.

Will this cut off DMA for all active downstream devices, potentially leading
to UR errors or system crashes?

[ ... ]

> @@ -1045,6 +1046,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev)

[Severity: High]
This isn't a bug introduced by this patch, but will unmasking hardware
interrupts earlier in shpc_init() allow the ISR to run before the slot list
is populated?

Hardware interrupts are unmasked before the calling function shpc_probe()
has a chance to execute init_slots():

drivers/pci/hotplug/shpchp_hpc.c:shpc_init() {
    ...
	for (hp_slot = 0; hp_slot < ctrl->num_slots; hp_slot++) {
		slot_reg = shpc_readl(ctrl, SLOT_REG(hp_slot));
		...
		slot_reg &= ~(PRSNT_CHANGE_INTR_MASK | ISO_PFAULT_INTR_MASK | ...);
		shpc_writel(ctrl, SLOT_REG(hp_slot), slot_reg);
	}
    ...
}

If a hardware event is already pending, shpc_isr() executes immediately and
calls shpchp_handle_presence_change().

Since shpchp_find_slot() would return a NULL p_slot on an empty list, won't
this cause an immediate NULL pointer dereference when accessing
p_slot->presence_save?

drivers/pci/hotplug/shpchp_ctrl.c:shpchp_handle_presence_change() {
    ...
	p_slot = shpchp_find_slot(ctrl, hp_slot + ctrl->slot_device_offset);

	/* Save the presence state */
	shpchp_get_adapter_status(p_slot, &p_slot->presence_save);
    ...
}

>  	iounmap(ctrl->creg);
>  abort_release_region:
>  	release_mem_region(ctrl->mmio_base, ctrl->mmio_size);
> +abort_disable_device:
> +	pci_disable_device(pdev);

[Severity: High]
Similarly, might disabling the bridge on initialization failure cause problems
if child devices were already active and relying on the bridge?

>  abort:
>  	return rc;
>  }

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