Re: [PATCH v3] PCI: pnv_php: Add null checks for OpenCAPI PHBs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] NULL pointer dereference in `pnv_php_alloc_slot()` during error handling.
- [High] Lockless iteration over PCI bus lists risks memory corruption.
- [High] Data race on `php_slot->irq` can lead to a double `free_irq()`.
- [High] Lockless modification of `php_slot->state` leads to TOCTOU races.
--

commit 72170284e1b2c84d6ca9a8f9f30c4d6aaa2d9c12
Author: Aditya Gupta <adityag@linux.ibm.com>

PCI: pnv_php: Add null checks for OpenCAPI PHBs

For OpenCAPI phb direct slots, the .pdev for php_slots will be NULL.
Various sections of the code in pnv_php can do a null dereference and
crash the kernel. Add null checks to prevent the null dereferences.

Cc: stable@vger.kernel.org
Fixes: 80f9fc236279 ("PCI: pnv_php: Work around switches with broken presence detection")

> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index ff92a5c301b81..d0f5e8ad1f716 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -47,6 +47,9 @@ static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
>  	struct pci_dev *pdev = php_slot->pdev;
>  	u16 ctrl;
>  
> +	if (!pdev)
> +		return;
> +
>  	if (php_slot->irq > 0) {
>  		pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
>  		ctrl &= ~(PCI_EXP_SLTCTL_HPIE |
>  			  PCI_EXP_SLTCTL_PDCE |
>  			  PCI_EXP_SLTCTL_DLLSCE);
>  		pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);
>  
>  		free_irq(php_slot->irq, php_slot);
>  		php_slot->irq = 0;
>  	}

[Severity: High]
This is a pre-existing issue, but can the lockless check of php_slot->irq
lead to a double free?

If two threads execute pnv_php_disable_irq() concurrently (such as from
multiple sysfs writes on parent and child slots), could they both observe
php_slot->irq > 0 and attempt to call free_irq() for the same IRQ before
it is cleared?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does pnv_php_alloc_slot() crash if
alloc_workqueue() fails?

drivers/pci/hotplug/pnv_php.c:pnv_php_alloc_slot() {
	...
	if (!php_slot->wq) {
		SLOT_WARN(php_slot, "Cannot alloc workqueue\n");
	...
}

If SLOT_WARN is invoked here, php_slot->bus is not yet initialized. The
SLOT_WARN macro evaluates to dev_warn(&(sl)->bus->dev, ...), which would take
the address of dev inside a NULL bus pointer. Could this trigger a kernel
Oops when dev_warn() attempts to read the device name?

[Severity: High]
This is a pre-existing issue, but is it safe to traverse bus->children and
bus->slots without acquiring pci_bus_sem?

drivers/pci/hotplug/pnv_php.c:pnv_php_disable_all_irqs() {
	...
	/* First go down child buses */
	list_for_each_entry(child_bus, &bus->children, node)
		pnv_php_disable_all_irqs(child_bus);

	/* Disable IRQs for all pnv_php slots on this bus */
	list_for_each_entry(slot, &bus->slots, list) {
	...
}

Could a concurrent PCI bus rescan or device hot-unplug race with this
lockless traversal, leading to list corruption or a NULL pointer dereference?

[Severity: High]
This is a pre-existing issue, but is there a race condition when checking
php_slot->state locklessly?

drivers/pci/hotplug/pnv_php.c:pnv_php_enable() {
	...
	if (php_slot->state != PNV_PHP_STATE_REGISTERED)
		return 0;
	...
}

And in a concurrent path:

drivers/pci/hotplug/pnv_php.c:pnv_php_unregister_one() {
	...
	php_slot->state = PNV_PHP_STATE_OFFLINE;
	pci_hp_deregister(&php_slot->slot);
	...
}

If a concurrent driver unbind calls pnv_php_unregister_one(), it sets the
state to PNV_PHP_STATE_OFFLINE. If an asynchronous hotplug event executing
pnv_php_enable() has already passed the PNV_PHP_STATE_REGISTERED check,
could it erroneously operate on a slot that is being destroyed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609084903.1352581-1-adityag@linux.ibm.com?part=1