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 > > This works with every type of interface I've tried. It appears that > if_clone_destroy and if_clone_create race with other ioctls, which > causes a variety of different UaFs or just general logic errors.
Thanks for the report. This is a long standing & known issue. > One common root cause appears to be that most ifioctl functions use > ifunit() to find an interface by name, which traverses if_list. Writes > to if_list are protected by a lock, but reads are apparently > unprotected. There's also the question of the life time of the object > returned from ifunit(). Most things that access &ifnet's if_list are > done without locking, and even if those accesses were to be locked, the > lock would be released before the object is no longer used, causing the > UaF in that case as well. Any sleeping point between the first ifunit() and the end of `ifc_create' or `ifc_destroy' respectively can lead to races. > This patch fixes the issue by making if_clone_{create,destroy} exclusive > with all other ifioctls. Diff below achieves the same but moves the locking inside the if_clone* functions such that consumers like tun(4), bridge(4) and switch(4) which clone interfaces upon open(2) are also serialized. I also added a NET_ASSERT_UNLOCKED() in both functions because the new lock must be grabbed before the NET_LOCK() in order to allow ifc_create and ifc_destroy to manipulate data structures protected by this lock. Comments, ok? Index: net/if.c =================================================================== RCS file: /cvs/src/sys/net/if.c,v retrieving revision 1.610 diff -u -p -r1.610 if.c --- net/if.c 22 Jun 2020 09:45:13 -0000 1.610 +++ net/if.c 23 Jun 2020 10:25:41 -0000 @@ -224,6 +224,7 @@ void if_idxmap_remove(struct ifnet *); TAILQ_HEAD(, ifg_group) ifg_head = TAILQ_HEAD_INITIALIZER(ifg_head); +struct rwlock if_clone_lock = RWLOCK_INITIALIZER("if_clone_lock"); LIST_HEAD(, if_clone) if_cloners = LIST_HEAD_INITIALIZER(if_cloners); int if_cloners_count; @@ -1246,24 +1247,36 @@ if_clone_create(const char *name, int rd struct ifnet *ifp; int unit, ret; - ifc = if_clone_lookup(name, &unit); - if (ifc == NULL) - return (EINVAL); + NET_ASSERT_UNLOCKED(); + rw_enter_write(&if_clone_lock); - if (ifunit(name) != NULL) - return (EEXIST); + ifc = if_clone_lookup(name, &unit); + if (ifc == NULL) { + ret = EINVAL; + goto out; + } + if (ifunit(name) != NULL) { + ret = EEXIST; + goto out; + } ret = (*ifc->ifc_create)(ifc, unit); + if (ret != 0) + goto out; - if (ret != 0 || (ifp = ifunit(name)) == NULL) - return (ret); + ifp = ifunit(name); + if (ifp == NULL) { + ret = EAGAIN; + goto out; + } NET_LOCK(); if_addgroup(ifp, ifc->ifc_name); if (rdomain != 0) if_setrdomain(ifp, rdomain); NET_UNLOCK(); - +out: + rw_exit_write(&if_clone_lock); return (ret); } @@ -1277,16 +1290,23 @@ if_clone_destroy(const char *name) struct ifnet *ifp; int ret; - ifc = if_clone_lookup(name, NULL); - if (ifc == NULL) - return (EINVAL); + NET_ASSERT_UNLOCKED(); + rw_enter_write(&if_clone_lock); + ifc = if_clone_lookup(name, NULL); + if (ifc == NULL) { + ret = EINVAL; + goto out; + } ifp = ifunit(name); - if (ifp == NULL) - return (ENXIO); - - if (ifc->ifc_destroy == NULL) - return (EOPNOTSUPP); + if (ifp == NULL) { + ret = ENXIO; + goto out; + } + if (ifc->ifc_destroy == NULL) { + ret = EOPNOTSUPP; + goto out; + } NET_LOCK(); if (ifp->if_flags & IFF_UP) { @@ -1297,7 +1317,8 @@ if_clone_destroy(const char *name) } NET_UNLOCK(); ret = (*ifc->ifc_destroy)(ifp); - +out: + rw_exit_write(&if_clone_lock); return (ret); }