Skip to contentSkip to conversation

PCI dynamic id use after free?

DISCUSSION Stephen Hemminger 2 participants 2 messages Apr 7, 2016 PCI

lore
Share & tools
lore
#0 lore raw

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

#1 lore raw
Quoted 23 lines · 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);
Quoted 4 lines

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

Quoted 1 line

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