Re: [PATCH v4 7/9] PCI: Make pci_match_one_device() match on ID instead of device

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free in PCI device probe due to unprotected dynamic ID pointer.
--

--- Patch [7]: [PATCH v4 7/9] PCI: Make pci_match_one_device() match on ID instead of device ---
commit 6e87a7c82585926a2bb69878be8d34b27c58a215
Author: Gary Guo <gary@garyguo.net>

PCI: Make pci_match_one_device() match on ID instead of device

PCI dynamic ID needs to match IDs against a new ID to see if it already
exists. Existing APIs can only match IDs against devices, causing the
dynamic ID insertion code to create a temporary device only for matching
purpose. Rename pci_match_one_device() to pci_match_one_id() so it can be
used for this purpose instead; add a pci_id_from_device() helper to make it
easy to convert users.

> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f36778e62ac1a..c9424edb45481 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c

[ ... ]

> @@ -145,10 +162,11 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>  	if (ret == 0)
>  		return NULL;
>  
> +	dev_id = pci_id_from_device(dev);
>  	/* Look at the dynamic ids first, before the static ones */
>  	spin_lock(&drv->dynids.lock);
>  	list_for_each_entry(dynid, &drv->dynids.list, node) {
> -		if (pci_match_one_device(&dynid->id, dev)) {
> +		if (pci_match_one_id(&dynid->id, &dev_id)) {
>  			found_id = &dynid->id;
>  			break;
>  		}

[ ... ]

> @@ -158,7 +176,7 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>  	if (found_id)
>  		return found_id;

[Severity: High]
This isn't a bug introduced by this patch, but could this returned found_id
pointer lead to a use-after-free?

Looking at pci_match_device(), if it matches a dynamic ID, it sets found_id
to &dynid->id and then drops drv->dynids.lock before returning the pointer.

If a concurrent write to the driver's remove_id sysfs attribute occurs,
remove_id_store() will acquire the same lock, remove the dynamic ID from the
list, and free it:

drivers/pci/pci-driver.c:remove_id_store() {
	...
			list_del(&dynid->node);
			kfree(dynid);
			retval = count;
			break;
	...
}

Wouldn't the probe path then pass this dangling pointer to the driver's probe
function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-pci_id_fix-v4-0-3580726844e1@garyguo.net?part=7