DF-2679 / fix.diff
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 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 | --- 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); } |