DF-3280 / fix_validation.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 147 148 149 150 151 152 153 154 155 | --- if.c.orig 2026-09-10 02:34:29.419130783 +0000 +++ if.c.fixed 2026-09-10 02:51:59.901641114 +0000 @@ -131,7 +131,6 @@ const struct ifnet_array *); static struct ifnet_array *ifnet_array_del(struct ifnet *, const struct ifnet_array *); -static struct ifg_group *if_creategroup(const char *); static int if_destroygroup(struct ifg_group *); static int if_delgroup_locked(struct ifnet *, const char *); static int if_getgroups(struct ifgroupreq *, struct ifnet *); @@ -1102,22 +1101,23 @@ /* * Create an empty interface group. */ +/* + * NOTE: ifgroup_lock must be held exclusively when calling this. + */ static struct ifg_group * -if_creategroup(const char *groupname) +if_creategroup_locked(const char *groupname) { struct ifg_group *ifg; + KKASSERT(lockstatus(&ifgroup_lock, curthread) == LK_EXCLUSIVE); + ifg = kmalloc(sizeof(*ifg), M_IFNET, M_WAITOK); strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group)); ifg->ifg_refcnt = 0; ifg->ifg_carp_demoted = 0; TAILQ_INIT(&ifg->ifg_members); - ifgroup_lockmgr(LK_EXCLUSIVE); TAILQ_INSERT_TAIL(&ifg_head, ifg, ifg_next); - ifgroup_lockmgr(LK_RELEASE); - - EVENTHANDLER_INVOKE(group_attach_event, ifg); return (ifg); } @@ -1148,21 +1148,32 @@ int if_addgroup(struct ifnet *ifp, const char *groupname) { - struct ifg_list *ifgl; + struct ifg_list *ifgl, *ifgl_tmp; struct ifg_group *ifg; struct ifg_member *ifgm; + int created = 0; + int error; if (groupname[0] && groupname[strlen(groupname) - 1] >= '0' && groupname[strlen(groupname) - 1] <= '9') return (EINVAL); - ifgroup_lockmgr(LK_SHARED); + ifgl = kmalloc(sizeof(*ifgl), M_IFNET, M_WAITOK); + ifgm = kmalloc(sizeof(*ifgm), M_IFNET, M_WAITOK); - TAILQ_FOREACH(ifgl, &ifp->if_groups, ifgl_next) { - if (strcmp(ifgl->ifgl_group->ifg_group, groupname) == 0) { - ifgroup_lockmgr(LK_RELEASE); - return (EEXIST); + ifgroup_lockmgr(LK_EXCLUSIVE); + + /* + * NOTE: Group lookup, creation, and the refcount bump are + * performed atomically w.r.t. other if_addgroup() calls and + * w.r.t. if_delgroup_locked() -- ifg_refcnt is modified by + * both, so the increment must be covered by the ifgroup lock. + */ + TAILQ_FOREACH(ifgl_tmp, &ifp->if_groups, ifgl_next) { + if (strcmp(ifgl_tmp->ifgl_group->ifg_group, groupname) == 0) { + error = EEXIST; + goto failed; } } @@ -1170,26 +1181,31 @@ if (strcmp(ifg->ifg_group, groupname) == 0) break; } + if (ifg == NULL) { + ifg = if_creategroup_locked(groupname); + created = 1; + } - ifgroup_lockmgr(LK_RELEASE); - - if (ifg == NULL) - ifg = if_creategroup(groupname); - - ifgl = kmalloc(sizeof(*ifgl), M_IFNET, M_WAITOK); - ifgm = kmalloc(sizeof(*ifgm), M_IFNET, M_WAITOK); ifgl->ifgl_group = ifg; ifgm->ifgm_ifp = ifp; ifg->ifg_refcnt++; - ifgroup_lockmgr(LK_EXCLUSIVE); TAILQ_INSERT_TAIL(&ifg->ifg_members, ifgm, ifgm_next); TAILQ_INSERT_TAIL(&ifp->if_groups, ifgl, ifgl_next); + ifgroup_lockmgr(LK_RELEASE); + if (created) + EVENTHANDLER_INVOKE(group_attach_event, ifg); EVENTHANDLER_INVOKE(group_change_event, groupname); return (0); + +failed: + ifgroup_lockmgr(LK_RELEASE); + kfree(ifgl, M_IFNET); + kfree(ifgm, M_IFNET); + return (error); } /* @@ -1348,6 +1364,36 @@ return (ENOMEM); ifgroup_lockmgr(LK_SHARED); + + /* + * The group may have been destroyed (or destroyed and + * recreated) while we were not holding the lock. Look it up + * again rather than dereferencing the potentially stale + * pointer found above. + */ + TAILQ_FOREACH(ifg, &ifg_head, ifg_next) { + if (strcmp(ifg->ifg_group, ifgr->ifgr_name) == 0) + break; + } + if (ifg == NULL) { + ifgroup_lockmgr(LK_RELEASE); + kfree(ifgrq, M_TEMP); + return (ENOENT); + } + + /* + * Membership may also have changed; recompute the length and + * re-check it against the caller's buffer. + */ + len = 0; + TAILQ_FOREACH(ifgm, &ifg->ifg_members, ifgm_next) + len += sizeof(struct ifg_req); + if (ifgr->ifgr_len != len) { + ifgroup_lockmgr(LK_RELEASE); + kfree(ifgrq, M_TEMP); + return (EINVAL); + } + p = ifgrq; TAILQ_FOREACH(ifgm, &ifg->ifg_members, ifgm_next) { if (len < sizeof(struct ifg_req)) { |