PCI dynamic id use after free?

PCI dynamic id use after free?

[Stephen Hemminger]

I was looking at another PCI problem, and discovered this potential use
after kfree.

static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
						    struct pci_dev *dev)
{
	struct pci_dynid *dynid;
	const struct pci_device_id *found_id = NULL;

	/* When driver_override is set, only bind to the matching driver */
	if (dev->driver_override && strcmp(dev->driver_override, drv->name))
		return NULL;

	/* 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)) {
			found_id = &dynid->id;
			break;
		}
	}
	spin_unlock(&drv->dynids.lock);

At this point found_id if matched (points into dynid) structure but the
lock has been dropped.

What prevents the ID from being removed by store_remvoe_id?

Looks like you need RCU (or ref counts here).

Re: PCI dynamic id use after free?

[Greg KH]

On Thu, Apr 07, 2016 at 10:15:59AM -0700, Stephen Hemminger wrote:
> I was looking at another PCI problem, and discovered this potential use
> after kfree.
> 
> static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
> 						    struct pci_dev *dev)
> {
> 	struct pci_dynid *dynid;
> 	const struct pci_device_id *found_id = NULL;
> 
> 	/* When driver_override is set, only bind to the matching driver */
> 	if (dev->driver_override && strcmp(dev->driver_override, drv->name))
> 		return NULL;
> 
> 	/* 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)) {
> 			found_id = &dynid->id;
> 			break;
> 		}
> 	}
> 	spin_unlock(&drv->dynids.lock);
> 
> At this point found_id if matched (points into dynid) structure but the
> lock has been dropped.
> 
> What prevents the ID from being removed by store_remvoe_id?

Nothing, good catch.  Luckily no one actually uses that interface :)

> Looks like you need RCU (or ref counts here).

Yes, or create a copy of the id and use that for the short period it is
in use.  I don't have the time at the moment to fix this until next
week, if someone wants to write up a patch before then... :)

thanks,

greg k-h