DragonFlyBSD Kernel Audit
DF-2679 / fix.diff
← back to finding ↓ download raw
--- a/sys/kern/subr_bus.c	2026-08-30 14:48:47.166450905 +0000
+++ b/sys/kern/subr_bus.c	2026-08-30 15:10:34.621697130 +0000
@@ -58,6 +58,20 @@
 
 MALLOC_DEFINE(M_BUS, "bus", "Bus data structures");
 
+/*
+ * DF-2679: the device topology (bus_data_devices list contents and the
+ * fields of each device_t published through it) is mutated without any
+ * synchronization while unprivileged sysctl readers (sysctl_devices,
+ * device_sysctl_handler) walk and dereference it.  Serialize the
+ * unpublish/free side against those readers with a global sleepable
+ * lock.  Readers take it shared only while walking/snapshotting device
+ * state (the lock is dropped before any sleeping copyout); teardown
+ * paths that unlink and free a device_t, and the paths that free
+ * dev->nameunit, take it exclusive.  Publication (make_device) is not
+ * locked to avoid lock-order inversions with bus-driver attach paths.
+ */
+static struct lock bus_topo_lock;
+
 #ifdef BUS_DEBUG
 #define PDEBUG(a)	(kprintf("%s:%d: ", __func__, __LINE__), kprintf a, kprintf("\n"))
 #define DEVICENAME(d)	((d)? device_get_name(d): "no device")
@@ -157,7 +171,16 @@
 	char *buf;
 	int error;
 
+	char lbuf[1024];
+
 	buf = NULL;
+	/*
+	 * Snapshot the string under the shared topology lock (dev, its
+	 * nameunit/desc and its parent cannot be freed while we hold it),
+	 * then drop the lock before the potentially sleeping copyout
+	 * (DF-2679).
+	 */
+	lockmgr(&bus_topo_lock, LK_SHARED);
 	switch (arg2) {
 	case DEVICE_SYSCTL_DESC:
 		value = dev->desc ? dev->desc : "";
@@ -177,11 +200,14 @@
 		value = dev->parent ? dev->parent->nameunit : "";
 		break;
 	default:
+		lockmgr(&bus_topo_lock, LK_RELEASE);
 		return (EINVAL);
 	}
-	error = SYSCTL_OUT(req, value, strlen(value));
+	strlcpy(lbuf, value, sizeof(lbuf));
+	lockmgr(&bus_topo_lock, LK_RELEASE);
 	if (buf != NULL)
 		kfree(buf, M_BUS);
+	error = SYSCTL_OUT(req, lbuf, strlen(lbuf));
 	return (error);
 }
 
@@ -314,6 +340,7 @@
 {
 	lockinit(&devsoftc.lock, "dev mtx", 0, 0);
 	TAILQ_INIT(&devsoftc.devq);
+	lockinit(&bus_topo_lock, "bustopo", 0, 0);
 }
 SYSINIT(predevinit, SI_SUB_CREATE_INIT, SI_ORDER_ANY, predevinit, 0);
 
@@ -1299,11 +1326,19 @@
 			return(error);
 	}
 
+	/*
+	 * Unpublish and free the device under the topology lock so that
+	 * sysctl_devices() / device_sysctl_handler() readers cannot hold a
+	 * pointer to it across the kobj_delete() (DF-2679).
+	 * devclass_delete_device() also frees dev->nameunit.
+	 */
+	lockmgr(&bus_topo_lock, LK_EXCLUSIVE);
 	if (child->devclass)
 		devclass_delete_device(child->devclass, child);
 	TAILQ_REMOVE(&dev->children, child, link);
 	TAILQ_REMOVE(&bus_data_devices, child, devlink);
 	kobj_delete((kobj_t)child, M_BUS);
+	lockmgr(&bus_topo_lock, LK_RELEASE);
 
 	bus_data_generation_update();
 	return(0);
@@ -1911,8 +1946,12 @@
 	int error;
 
 	if (!classname) {
-		if (dev->devclass)
+		if (dev->devclass) {
+			/* frees dev->nameunit: exclude sysctl readers */
+			lockmgr(&bus_topo_lock, LK_EXCLUSIVE);
 			devclass_delete_device(dev->devclass, dev);
+			lockmgr(&bus_topo_lock, LK_RELEASE);
+		}
 		return(0);
 	}
 
@@ -2143,8 +2182,12 @@
 	if (dev->parent)
 		BUS_CHILD_DETACHED(dev->parent, dev);
 
-	if (!(dev->flags & DF_FIXEDCLASS))
+	if (!(dev->flags & DF_FIXEDCLASS)) {
+		/* frees dev->nameunit: exclude sysctl readers (DF-2679) */
+		lockmgr(&bus_topo_lock, LK_EXCLUSIVE);
 		devclass_delete_device(dev->devclass, dev);
+		lockmgr(&bus_topo_lock, LK_RELEASE);
+	}
 
 	dev->state = DS_NOTPRESENT;
 	device_set_driver(dev, NULL);
@@ -3874,14 +3917,24 @@
 	index = name[1];
 
 	/*
+	 * Walk and snapshot the device under the shared topology lock so a
+	 * concurrent device_delete_child() cannot free the entry we are on
+	 * (DF-2679).  The lock is dropped before the (possibly sleeping)
+	 * copyout.  The generation check above is only advisory.
+	 */
+	lockmgr(&bus_topo_lock, LK_SHARED);
+
+	/*
 	 * Scan the list of devices, looking for the requested index.
 	 */
 	TAILQ_FOREACH(dev, &bus_data_devices, devlink) {
 		if (index-- == 0)
 			break;
 	}
-	if (dev == NULL)
+	if (dev == NULL) {
+		lockmgr(&bus_topo_lock, LK_RELEASE);
 		return (ENOENT);
+	}
 
 	/*
 	 * Populate the return array.
@@ -3901,6 +3954,7 @@
 	udev.dv_devflags = dev->devflags;
 	udev.dv_flags = dev->flags;
 	udev.dv_state = dev->state;
+	lockmgr(&bus_topo_lock, LK_RELEASE);
 	error = SYSCTL_OUT(req, &udev, sizeof(udev));
 	return (error);
 }