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
