⬢ DragonFlyBSD Kernel Audit
DF-5101 / fix.diff
← back to finding ↓ download raw 
  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
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);
 }