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)