⬢ DragonFlyBSD Kernel Audit
DF-3281 / fix.diff
← back to finding ↓ download raw
--- if.c.orig	2026-09-10 02:34:29.419130783 +0000
+++ if.c.fixed	2026-09-10 02:35:58.185986949 +0000
@@ -1102,19 +1102,34 @@
 /*
  * 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);
+
+	return (ifg);
+}
+
+static struct ifg_group *
+if_creategroup(const char *groupname)
+{
+	struct ifg_group *ifg;
+
+	ifgroup_lockmgr(LK_EXCLUSIVE);
+	ifg = if_creategroup_locked(groupname);
 	ifgroup_lockmgr(LK_RELEASE);
 
 	EVENTHANDLER_INVOKE(group_attach_event, ifg);
@@ -1148,21 +1163,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 +1196,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);
 }
 
 /*