Re: [PATCH] PCI: fix use-after-free in pci_pme_list_scan()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional list addition and reference acquisition in `pci_pme_active()` creates duplicate entries, leading to a permanent memory leak of `pci_dev`.
--

commit 441d645c73e4a6fdd2c7556f46a08da0b787984e
Author: Torsten Hilbrich <torsten.hilbrich@secunet.com>

PCI: fix use-after-free in pci_pme_list_scan()

This commit addresses a use-after-free in pci_pme_list_scan() by taking a
reference to the pci_dev for each list entry in pci_pme_active() and
dropping it when the entry is removed. This ensures the device is kept
alive as long as its entry is on the list.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f80..b3a4b710d7c8d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> @@ -2497,7 +2498,7 @@ void pci_pme_active(struct pci_dev *dev, bool enable)
>  				pci_warn(dev, "can't enable PME#\n");
>  				return;
>  			}
> -			pme_dev->dev = dev;
> +			pme_dev->dev = pci_dev_get(dev);
>  			mutex_lock(&pci_pme_list_mutex);
>  			list_add(&pme_dev->list, &pci_pme_list);

[Severity: High]
Does this unconditional list addition and reference acquisition create duplicate
entries, leading to a permanent memory leak of pci_dev?

For example, during suspend, __pci_enable_wake() calls pci_pme_active(dev,
true), which allocates an entry, takes a pci_dev_get() reference, and adds it
to the list. If the subsequent platform_pci_set_wakeup() call fails,
__pci_enable_wake() exits without setting dev->wakeup_prepared = true.

On resume, __pci_enable_wake() returns immediately because wakeup_prepared is
false, meaning it never calls pci_pme_active(dev, false) to remove the entry.
On the next suspend cycle, pci_pme_active(dev, true) would add a second
duplicate entry for the same device, taking another pci_dev_get() reference.

>  			if (list_is_singular(&pci_pme_list))
> @@ -2510,6 +2511,7 @@ void pci_pme_active(struct pci_dev *dev, bool enable)
>  			list_for_each_entry(pme_dev, &pci_pme_list, list) {
>  				if (pme_dev->dev == dev) {
>  					list_del(&pme_dev->list);
> +					pci_dev_put(pme_dev->dev);
>  					kfree(pme_dev);
>  					break;
>  				}

[Severity: High]
Because of the break statement here, only one entry is removed during device
teardown via pci_pme_active(dev, false).

If duplicate entries accumulated (e.g. from the __pci_enable_wake() failure
scenario mentioned above, or if pdev->pme_poll is cleared and reset by a
driver), the remaining entries will stay on the list permanently. Since this
patch introduces pci_dev_get() on addition, these lingering entries will hold
permanent references to the pci_dev, preventing it from being freed by
pci_destroy_dev().

Could we check if the device is already on the list before adding it, or handle
duplicates during removal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/f2e43015-19ca-4e58-9b86-bdd855ff6496@secunet.com?part=1