DragonFlyBSD Kernel Audit
DF-0725 / fix.diff
← back to finding ↓ download raw
diff --git a/sys/net/if_clone.c b/sys/net/if_clone.c
--- a/sys/net/if_clone.c
+++ b/sys/net/if_clone.c
@@ -107,23 +107,42 @@
 	struct ifnet *ifp;
 	int unit, error;
 
+	/*
+	 * Hold ifnet_lock across ifunit() AND all accesses of the returned
+	 * ifp, as required by the MPSAFE contract in if_var.h.  Previously
+	 * the lock was dropped right after ifunit() and re-acquired only
+	 * around if_clone_free_unit()/ifc_destroy(); two concurrent
+	 * destroyers could both observe a live ifp and then both enter the
+	 * critical section, double-clearing the unit bitmap (KKASSERT at
+	 * if_clone_free_unit) and racing the ifp free against the second
+	 * destroyer's ifp deref / ifc_destroy(ifp) call (UAF).  ifc_destroy()
+	 * already runs under ifnet_lock in the original code and ifnet_mtx is
+	 * recursive, so extending the hold over the derefs introduces no new
+	 * lock-ordering constraint.
+	 */
 	ifnet_lock();
 	ifp = ifunit(name);
-	ifnet_unlock();
-	if (ifp == NULL)
+	if (ifp == NULL) {
+		ifnet_unlock();
 		return (ENXIO);
+	}
 
-	if ((ifc = if_clone_lookup(ifp->if_dname)) == NULL)
+	if ((ifc = if_clone_lookup(ifp->if_dname)) == NULL) {
+		ifnet_unlock();
 		return (EINVAL);
+	}
 
 	unit = ifp->if_dunit;
-	if (unit < ifc->ifc_minifs)
+	if (unit < ifc->ifc_minifs) {
+		ifnet_unlock();
 		return (EINVAL);
+	}
 
-	if (ifc->ifc_destroy == NULL)
+	if (ifc->ifc_destroy == NULL) {
+		ifnet_unlock();
 		return (EOPNOTSUPP);
+	}
 
-	ifnet_lock();
 	if_clone_free_unit(ifc, unit);
 	error = ifc->ifc_destroy(ifp);
 	if (error)