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);
}
