Re: [PATCH v3 9/9] pci: fix UAF when probe runs concurrent to dyn ID removal

[Bjorn Helgaas]

On Mon, Jul 06, 2026 at 03:11:21PM +0100, Gary Guo wrote:
> Dynamic IDs are only guaranteed to be valid when dynids.lock is held,
> as remove_id_store can free the node. Thus, make a copy in
> pci_match_device. Also, clarify that the id parameter is only valid during
> probe.
> 
> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> Link: https://lore.kernel.org/all/20260619170503.518F61F00A3A@xxxxxxxxxxxxxxx/
> Fixes: 0994375e9614 ("PCI: add remove_id sysfs entry")
> Signed-off-by: Gary Guo <gary@xxxxxxxxxxx>
> ---
>  drivers/pci/pci-driver.c | 28 +++++++++++++++-------------
>  include/linux/pci.h      |  1 +
>  2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 2e80ae150ff4..4851061babcb 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -179,6 +179,7 @@ static const struct pci_device_id pci_device_id_any = {
>   * pci_match_device - See if a device matches a driver's list of IDs
>   * @drv: the PCI driver to match against
>   * @dev: the PCI device structure to match against
> + * @id_copy: Place to store copy of pci_device_id for dynamic ID

s/Place/place/ (capitalize same as the others)

>   * Used by a driver to check whether a PCI device is in its list of
>   * supported devices or in the dynids list, which may have been augmented
> @@ -186,9 +187,9 @@ static const struct pci_device_id pci_device_id_any = {
>   * structure or %NULL if there is no match.
>   */
>  static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
> -						    struct pci_dev *dev)
> +						    struct pci_dev *dev,
> +						    struct pci_device_id *id_copy)
>  {
> -	struct pci_dynid *dynid;
>  	const struct pci_device_id *found_id = NULL;
>  	struct pci_device_id dev_id;
>  	int ret;
> @@ -200,17 +201,16 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>  
>  	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_id(&dynid->id, &dev_id)) {
> -			found_id = &dynid->id;
> -			break;
> +	scoped_guard(spinlock, &drv->dynids.lock) {
> +		struct pci_dynid *dynid;
> +
> +		list_for_each_entry(dynid, &drv->dynids.list, node) {
> +			if (pci_match_one_id(&dynid->id, &dev_id)) {
> +				*id_copy = dynid->id;
> +				return id_copy;
> +			}
>  		}
>  	}
> -	spin_unlock(&drv->dynids.lock);
> -
> -	if (found_id)
> -		return found_id;
>  
>  	found_id = do_pci_match_id(drv->id_table, &dev_id, ret > 0);
>  	if (found_id)
> @@ -466,12 +466,13 @@ void pci_probe_flush_workqueue(void)
>  static int __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
>  {
>  	const struct pci_device_id *id;
> +	struct pci_device_id id_copy;
>  	int error = 0;
>  
>  	if (drv->probe) {
>  		error = -ENODEV;
>  
> -		id = pci_match_device(drv, pci_dev);
> +		id = pci_match_device(drv, pci_dev, &id_copy);
>  		if (id)
>  			error = pci_call_probe(drv, pci_dev, id);
>  	}
> @@ -1559,12 +1560,13 @@ static int pci_bus_match(struct device *dev, const struct device_driver *drv)
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	struct pci_driver *pci_drv;
>  	const struct pci_device_id *found_id;
> +	struct pci_device_id id_copy;
>  
>  	if (pci_dev_binding_disallowed(pci_dev))
>  		return 0;
>  
>  	pci_drv = (struct pci_driver *)to_pci_driver(drv);
> -	found_id = pci_match_device(pci_drv, pci_dev);
> +	found_id = pci_match_device(pci_drv, pci_dev, &id_copy);
>  	if (found_id)
>  		return 1;
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 64b308b6e61c..92c17c116de6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -979,6 +979,7 @@ struct module;
>   *		function returns zero when the driver chooses to
>   *		take "ownership" of the device or an error code
>   *		(negative number) otherwise.
> + *		The pci_device_id parameter is only valid during probe.

The probe function takes a pointer to a struct pci_device_id, so I
think the requirement is that the struct pci_device_id only *needs* to
be valid during .probe(), right, i.e., the PCI core probe path makes
its own copy of the ID and doesn't retain the pointer after .probe()
returns, right?

I assume the caller determines the struct pci_device_id lifetime, and
it could be forever.

Could say something like:

  The pci_device_id parameter only needs to be valid during probe.

Thanks for doing this work; it should fix a subtle but important
issue.

>   *		The probe function always gets called from process
>   *		context, so it can sleep.
>   * @remove:	The remove() function gets called whenever a device
> 
> -- 
> 2.54.0
>