On Thu, Jun 25, 2020 at 11:54:48AM +0200, Martin Pieuchot wrote: > On 23/06/20(Tue) 16:21, Martin Pieuchot wrote: > > On 23/06/20(Tue) 04:53, Jason A. Donenfeld wrote: > > > On 6/23/20, Martin Pieuchot <m...@openbsd.org> wrote: > > > > On 23/06/20(Tue) 01:00, Jason A. Donenfeld wrote: > > > >> You can crash a system by running something like: > > > >> > > > >> for i in 1 2 3; do while true; do ifconfig bridge0 create& ifconfig > > > >> bridge0 destroy& done& done > > As mentioned already the proposed diff doesn't protect creation of cloning > devices via open(2). The above test could be replaced by: > > for i in 1 2 3; do while true; \ > do cat /dev/tun0& ifconfig tun0 destroy& done& done > > The same could be applied to switch(4) or by replacing the cat(1) magic > with 'ifconfig bridge0 add vether0. >
I used this [1] diff with a little modification to tun(4). This modification is required because ifunit() doesn't know about reservaton used by if_clone_create(). Also I grab a reference on obtained `ifp'. I run the commands below. System is stable. ---- cut begin 1---- for i in 1 2 3; do while true; \ do ifconfig tun0 create& cat /dev/tun0& \ ifconfig tun0 destroy& done& done ---- cut end 1 ---- ---- cut begin 2---- ifconfig vether0 create for i in 1 2 3; do while true; do ifconfig bridge0 create& \ ifconfig bridge0 add vether0& \ ifconfig bridge0 destroy& done& done ---- cut end 2 ---- 1. https://marc.info/?l=openbsd-tech&m=159307900124245&w=2 Index: sys/net/if.c =================================================================== RCS file: /cvs/src/sys/net/if.c,v retrieving revision 1.610 diff -u -p -r1.610 if.c --- sys/net/if.c 22 Jun 2020 09:45:13 -0000 1.610 +++ sys/net/if.c 25 Jun 2020 11:53:42 -0000 @@ -1236,6 +1236,18 @@ if_isconnected(const struct ifnet *ifp0, return (connected); } +/* XXX: reserve unit */ +struct if_clone_unit { + LIST_ENTRY(if_clone_unit) ifcu_list; + struct if_clone *ifcu_ifc; + int ifcu_unit; +}; + +LIST_HEAD(, if_clone_unit) if_clone_units = + LIST_HEAD_INITIALIZER(if_clone_units); + +/* XXX: reserve unit */ + /* * Create a clone network interface. */ @@ -1246,6 +1258,8 @@ if_clone_create(const char *name, int rd struct ifnet *ifp; int unit, ret; + struct if_clone_unit *ifcu, *tifcu; + ifc = if_clone_lookup(name, &unit); if (ifc == NULL) return (EINVAL); @@ -1253,10 +1267,28 @@ if_clone_create(const char *name, int rd if (ifunit(name) != NULL) return (EEXIST); + /* XXX: reserve unit */ + ifcu=malloc(sizeof(*ifcu), M_TEMP, M_WAITOK | M_ZERO); + + LIST_FOREACH(tifcu, &if_clone_units, ifcu_list) { + if (tifcu->ifcu_ifc == ifc && tifcu->ifcu_unit == unit) { + free(ifcu, M_TEMP, 0); + return (EEXIST); + } + } + ifcu->ifcu_ifc = ifc; + ifcu->ifcu_unit = unit; + LIST_INSERT_HEAD(&if_clone_units, ifcu, ifcu_list); + /* XXX: reserve unit */ + + ret = (*ifc->ifc_create)(ifc, unit); - if (ret != 0 || (ifp = ifunit(name)) == NULL) + if (ret != 0 || (ifp = ifunit(name)) == NULL) { + LIST_REMOVE(ifcu, ifcu_list); + free(ifcu, M_TEMP, 0); return (ret); + } NET_LOCK(); if_addgroup(ifp, ifc->ifc_name); @@ -1275,15 +1307,18 @@ if_clone_destroy(const char *name) { struct if_clone *ifc; struct ifnet *ifp; - int ret; + int unit, ret; + + struct if_clone_unit *ifcu, *tifcu; - ifc = if_clone_lookup(name, NULL); + ifc = if_clone_lookup(name, &unit); if (ifc == NULL) return (EINVAL); ifp = ifunit(name); if (ifp == NULL) return (ENXIO); + ifp->if_dying = 1; if (ifc->ifc_destroy == NULL) return (EOPNOTSUPP); @@ -1298,6 +1333,15 @@ if_clone_destroy(const char *name) NET_UNLOCK(); ret = (*ifc->ifc_destroy)(ifp); + /* XXX: release unit */ + LIST_FOREACH_SAFE(ifcu, &if_clone_units, ifcu_list, tifcu) { + if (ifcu->ifcu_ifc == ifc && ifcu->ifcu_unit == unit) { + LIST_REMOVE(ifcu, ifcu_list); + free(ifcu, M_TEMP, 0); + } + } + /* XXX: release unit */ + return (ret); } @@ -1749,8 +1793,11 @@ ifunit(const char *name) KERNEL_ASSERT_LOCKED(); TAILQ_FOREACH(ifp, &ifnet, if_list) { - if (strcmp(ifp->if_xname, name) == 0) + if (strcmp(ifp->if_xname, name) == 0) { + if (ifp->if_dying) + ifp = NULL; return (ifp); + } } return (NULL); } @@ -1958,6 +2005,7 @@ ifioctl(struct socket *so, u_long cmd, c ifp = ifunit(ifr->ifr_name); if (ifp == NULL) return (ENXIO); + if_ref(ifp); oif_flags = ifp->if_flags; oif_xflags = ifp->if_xflags; @@ -2314,6 +2362,7 @@ ifioctl(struct socket *so, u_long cmd, c if (((oif_flags ^ ifp->if_flags) & IFF_UP) != 0) getmicrotime(&ifp->if_lastchange); + if_put(ifp); return (error); } @@ -2358,6 +2407,7 @@ ifioctl_get(u_long cmd, caddr_t data) ifp = ifunit(ifr->ifr_name); if (ifp == NULL) return (ENXIO); + if_ref(ifp); NET_RLOCK_IN_IOCTL(); @@ -2428,6 +2478,7 @@ ifioctl_get(u_long cmd, caddr_t data) } NET_RUNLOCK_IN_IOCTL(); + if_put(ifp); return (error); } @@ -2531,6 +2582,7 @@ ifconf(caddr_t data) if (space == 0) { TAILQ_FOREACH(ifp, &ifnet, if_list) { struct sockaddr *sa; + if_ref(ifp); if (TAILQ_EMPTY(&ifp->if_addrlist)) space += sizeof (ifr); @@ -2543,6 +2595,7 @@ ifconf(caddr_t data) sizeof(*sa); space += sizeof(ifr); } + if_put(ifp); } ifc->ifc_len = space; return (0); @@ -2550,6 +2603,7 @@ ifconf(caddr_t data) ifrp = ifc->ifc_req; TAILQ_FOREACH(ifp, &ifnet, if_list) { + if_ref(ifp); if (space < sizeof(ifr)) break; bcopy(ifp->if_xname, ifr.ifr_name, IFNAMSIZ); @@ -2589,6 +2643,7 @@ ifconf(caddr_t data) break; space -= sizeof (ifr); } + if_put(ifp); } ifc->ifc_len -= space; return (error); Index: sys/net/if_bridge.c =================================================================== RCS file: /cvs/src/sys/net/if_bridge.c,v retrieving revision 1.340 diff -u -p -r1.340 if_bridge.c --- sys/net/if_bridge.c 24 Jun 2020 22:03:42 -0000 1.340 +++ sys/net/if_bridge.c 25 Jun 2020 11:53:42 -0000 @@ -234,7 +234,9 @@ bridge_clone_destroy(struct ifnet *ifp) bstp_destroy(sc->sc_stp); /* Undo pseudo-driver changes. */ +#if 0 if_deactivate(ifp); +#endif if_ih_remove(ifp, ether_input, NULL); Index: sys/net/if_tun.c =================================================================== RCS file: /cvs/src/sys/net/if_tun.c,v retrieving revision 1.222 diff -u -p -r1.222 if_tun.c --- sys/net/if_tun.c 13 May 2020 00:48:06 -0000 1.222 +++ sys/net/if_tun.c 25 Jun 2020 11:53:42 -0000 @@ -381,8 +381,10 @@ tun_dev_open(dev_t dev, const struct if_ snprintf(name, sizeof(name), "%s%u", ifc->ifc_name, minor(dev)); rdomain = rtable_l2(p->p_p->ps_rtableid); +#if 0 /* let's find or make an interface to work with */ while ((ifp = ifunit(name)) == NULL) { + static u_int xxx=0; error = if_clone_create(name, rdomain); switch (error) { case 0: /* it's probably ours */ @@ -393,7 +395,18 @@ tun_dev_open(dev_t dev, const struct if_ default: return (error); } + printf("race %u\n", xxx++); } +#else +restart: + error = if_clone_create(name, rdomain); + + if( !(error == 0 || error == EEXIST)) + return (error); + if ((ifp = ifunit(name)) == NULL) + goto restart; + ifp = if_get(ifp->if_index); +#endif sc = ifp->if_softc; /* wait for it to be fully constructed before we use it */ @@ -401,12 +414,14 @@ tun_dev_open(dev_t dev, const struct if_ error = tsleep_nsec(sc, PCATCH, "tuninit", INFSLP); if (error != 0) { /* XXX if_clone_destroy if stayup? */ + if_put(ifp); return (error); } } if (sc->sc_dev != 0) { /* aww, we lost */ + if_put(ifp); return (EBUSY); } /* it's ours now */ @@ -475,6 +490,7 @@ tun_dev_close(dev_t dev, struct proc *p) sc->sc_dev = 0; tun_put(sc); + if_put(ifp); if (destroy) if_clone_destroy(name); Index: sys/net/if_var.h =================================================================== RCS file: /cvs/src/sys/net/if_var.h,v retrieving revision 1.105 diff -u -p -r1.105 if_var.h --- sys/net/if_var.h 12 May 2020 08:49:54 -0000 1.105 +++ sys/net/if_var.h 25 Jun 2020 11:53:42 -0000 @@ -185,6 +185,7 @@ struct ifnet { /* and the entries */ struct sockaddr_dl *if_sadl; /* [N] pointer to our sockaddr_dl */ void *if_afdata[AF_MAX]; + int if_dying; }; #define if_mtu if_data.ifi_mtu #define if_type if_data.ifi_type