PCI dynamic id use after free?
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).
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(structpci_driver*drv,
structpci_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_idif 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