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

[sashiko-bot@kernel.org]

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