DF-3407 / fix.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 | --- a/sys/net/tun/if_tun.c +++ b/sys/net/tun/if_tun.c @@ -283,15 +283,28 @@ if ((error = caps_priv_check(ap->a_cred, SYSCAP_RESTRICTEDROOT)) != 0) return (error); + ifnet_lock(); sc = dev->si_drv1; - if (sc == NULL && (sc = tuncreate(dev, 0)) == NULL) - return (ENOMEM); - if (sc->tun_flags & TUN_OPEN) + /* + * Do not resurrect: with the ifnet lock held, si_drv1 can only be + * NULL if tundestroy() already tore this unit down (both creation + * paths set si_drv1 under the lock, before the node is published). + * The dev may still be referenced/in-flight while destroy_dev() + * completes, so reference_dev() on it here would panic. + */ + if (sc == NULL) { + ifnet_unlock(); + return (ENXIO); + } + if (sc->tun_flags & TUN_OPEN) { + ifnet_unlock(); return (EBUSY); + } sc->tun_pid = curproc->p_pid; sc->tun_flags |= TUN_OPEN; tunrefcnt++; + ifnet_unlock(); TUNDEBUG(sc->tun_ifp, "opened, minor = %#x. Module refcnt = %d\n", minor(dev), tunrefcnt); @@ -305,11 +318,13 @@ tunclose(struct dev_close_args *ap) { cdev_t dev = ap->a_head.a_dev; - struct tun_softc *sc = dev->si_drv1; + struct tun_softc *sc; struct ifnet *ifp; int unit = minor(dev); char ifname[IFNAMSIZ]; + ifnet_lock(); + sc = dev->si_drv1; KASSERT(sc != NULL, ("try closing the already destroyed %s%d", TUN, unit)); ifp = sc->tun_ifp; @@ -349,8 +364,11 @@ /* Only auto-destroy if the interface was not manually created. */ if ((sc->tun_flags & TUN_MANUALMAKE) == 0) { ksnprintf(ifname, IFNAMSIZ, "%s%d", TUN, unit); - if_clone_destroy(ifname); + ifnet_unlock(); + if_clone_destroy(ifname); /* takes the ifnet lock */ + return (0); } + ifnet_unlock(); return (0); } @@ -883,6 +901,7 @@ if (error) { if (top) m_freem(top); + m_freem(m); /* spare MGET from the last iteration */ IFNET_STAT_INC(ifp, ierrors, 1); return (error); } --- a/sys/net/if_clone.c +++ b/sys/net/if_clone.c @@ -109,21 +109,34 @@ 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(); + /* + * Hold the ifnet lock across the lookup and the teardown: dropping + * it in between lets two concurrent destroyers of the same unit + * both observe the ifnet and double-run if_clone_free_unit() / + * ifc_destroy() (double-free of the clone unit, use-after-free of + * the ifnet). ifc_destroy was already called under this lock. + */ if_clone_free_unit(ifc, unit); error = ifc->ifc_destroy(ifp); if (error) |