⬢ DragonFlyBSD Kernel Audit
DF-3280 / fix_validation.diff
← back to finding ↓ download raw
--- 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)) {