Re: [PATCH v2 1/3] ppc/pnv: 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] Pre-existing NULL pointer dereference in the error path of `pnv_php_alloc_slot()` due to an uninitialized bus pointer.
- [High] Unbalanced `pci_enable_device()` refcount leak during setup failures and normal teardown.
- [High] Concurrent accesses to `pnv_php_enable_slot` and `pnv_php_disable_slot` can cause double-frees due to lack of state locking.
- [Medium] Improper error code in `pnv_php_reset_slot()` prematurely aborts PCI reset method discovery.
--

commit 534a90517227ad6163f935501ac2f0ed9cf60b83
Author: Aditya Gupta <adityag@linux.ibm.com>

ppc/pnv: Add null checks for OpenCapi PHBs

This patch adds null checks for php_slot->pdev across several PCI hotplug
operations in pnv_php.c. It prevents NULL pointer dereferences for OpenCAPI
PHB direct slots where the bus has no self device, fixing kernel crashes
observed during boot and hotplug events.

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

[Severity: High]
This is a pre-existing issue, but can concurrent accesses to the slot
operations cause a double-free here?

If pnv_php_disable_slot() is called concurrently by two threads (for example,
via sysfs writes to the power attribute racing with the workqueue event
handler), both threads can bypass the state check because php_slot->state is
not locked.

They could both execute pnv_php_disable_all_downstream_irqs() and reach
this point. Since both would see php_slot->irq > 0, they would both call
free_irq(php_slot->irq, php_slot).

Should there be some synchronization around the state transitions?

[ ... ]

> @@ -1093,6 +1107,9 @@ static void pnv_php_enable_irq(struct pnv_php_slot *php_slot)
>  	struct pci_dev *pdev = php_slot->pdev;
>  	int irq, ret;
>  
> +	if (!pdev)
> +		return;
> +
>  	/*
>  	 * The MSI/MSIx interrupt might have been occupied by other
>  	 * drivers. Don't populate the surprise hotplug capability
>  	 * in that case.
>  	 */
>  	if (pci_dev_msi_enabled(pdev))
>  		return;
>  
>  	ret = pci_enable_device(pdev);
>  	if (ret) {
>  		SLOT_WARN(php_slot, "Error %d enabling device\n", ret);
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does this leak the enable_cnt refcount?

pci_enable_device(pdev) increments the device's enable count. If the
subsequent IRQ initialization steps fail, this function returns without
calling pci_disable_device().

Additionally, during normal slot teardown, pnv_php_disable_irq() is called
with disable_device = false, meaning pci_disable_device() is skipped during
hot-unplug. This would permanently prevent the device from entering low-power
states and block clean driver unbinding.

Is there a missing pci_disable_device() in the error paths and unplug flow?

While reviewing this code, I also noticed a few other pre-existing issues in
this file:

drivers/pci/hotplug/pnv_php.c:pnv_php_reset_slot() {
    ...
	/*
	 * The CAPI folks want pnv_php to drive OpenCAPI slots
	 * which don't have a bridge. Only claim to support
	 * reset_slot() if we have a bridge device (for now...)
	 */
	if (probe)
		return !bridge;
    ...
}

[Severity: Medium]
This is a pre-existing issue, but does returning 1 here abort the PCI reset
method discovery loop prematurely?

The PCI core's __pci_reset_function_locked() expects -ENOTTY to indicate a
reset method is unsupported. When !bridge evaluates to 1, the PCI core sees
rc != -ENOTTY and aborts the entire loop, which prevents falling back to
other reset methods like FLR or PM reset.

---

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

[Severity: High]
This is a pre-existing issue, but could this SLOT_WARN cause a NULL pointer
dereference?

The php_slot structure is zero-initialized on allocation. If
alloc_workqueue() fails, SLOT_WARN evaluates php_slot->pdev (which is NULL)
and takes the false branch to use &php_slot->bus->dev.

However, php_slot->bus has not been assigned yet. Computing &(NULL)->dev
yields an invalid pointer offset that is passed to the underlying dev_warn()
function, which will crash when it attempts to dereference it.

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