Fix for DF-5101 — see findings/poc/DF-5101/VERDICT.md. Advisory only; never applied to sys/. --- a/sys/dev/drm/drm_irq.c +++ b/sys/dev/drm/drm_irq.c @@ -102,6 +102,7 @@ { int ret; unsigned long sh_flags = 0; + unsigned long irqflags; if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ)) return -EOPNOTSUPP; @@ -113,9 +114,14 @@ if (!dev->dev_private) return -EINVAL; - if (dev->irq_enabled) + /* Test-and-set atomically: callers hold different (or no) locks. */ + spin_lock_irqsave(&dev->vbl_lock, irqflags); + if (dev->irq_enabled) { + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); return -EBUSY; + } dev->irq_enabled = true; + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); DRM_DEBUG("irq=%d\n", irq); @@ -179,8 +185,15 @@ if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ)) return -EOPNOTSUPP; + /* Test-and-clear atomically: callers hold different (or no) locks. */ + spin_lock_irqsave(&dev->vbl_lock, irqflags); irq_enabled = dev->irq_enabled; dev->irq_enabled = false; + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); + + /* Bail out early if the handler was never installed. */ + if (!irq_enabled) + return -EINVAL; /* * Wake up any waiters so they don't hang. This is just to paper over @@ -204,9 +217,6 @@ spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } - if (!irq_enabled) - return -EINVAL; - DRM_DEBUG("irq=%d\n", dev->irq); #if 0 --- a/sys/dev/drm/linux_irq.c +++ b/sys/dev/drm/linux_irq.c @@ -111,20 +111,26 @@ struct drm_device *ddev = dev_id; device_t bsddev = ddev->dev->bsddev; struct resource *res = ddev->pdev->_irqr; - int found = 0; + /* + * Hold irqdata_lock across the lookup AND the removal (and remove + * BEFORE tearing down) so two racing free_irq() calls for the same + * (irq, dev_id) cannot both win the lookup and double-teardown / + * double-free the entry. + */ + lockmgr(&irqdata_lock, LK_EXCLUSIVE); SLIST_FOREACH_MUTABLE(irq_entry, &irq_list, id_irq_entries, tmp_ie) { - if ((irq_entry->irq == irq) && (irq_entry->dev_id == dev_id)) { - found = 1; + if ((irq_entry->irq == irq) && (irq_entry->dev_id == dev_id)) break; - } } - - if (!found) { + if (irq_entry == NULL) { + lockmgr(&irqdata_lock, LK_RELEASE); kprintf("free_irq: irq %d for dev_id %p was not registered\n", irq, dev_id); return; } + SLIST_REMOVE(&irq_list, irq_entry, irq_data, id_irq_entries); + lockmgr(&irqdata_lock, LK_RELEASE); bus_teardown_intr(bsddev, res, irq_entry->cookiep); bus_release_resource(bsddev, SYS_RES_IRQ, irq_entry->rid, res); @@ -133,10 +139,6 @@ ddev->pdev->_irq_type = 0; } ddev->pdev->_irqr = NULL; - - lockmgr(&irqdata_lock, LK_EXCLUSIVE); - SLIST_REMOVE(&irq_list, irq_entry, irq_data, id_irq_entries); - lockmgr(&irqdata_lock, LK_RELEASE); kfree(irq_entry); }