On 23/06/20(Tue) 04:53, Jason A. Donenfeld wrote:
> On 6/23/20, Martin Pieuchot <[email protected]> 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
> >>
> >> 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?
>
> Not okay. This is the first thing I tried, and it still races with
> ifioctl_get, causing UaF crashes. It's harder to trigger, but still
> possible after a few minutes of races. This structure here needs to be
> a read/write lock, which is what my original patch provides. (I guess
> I forgot to add the "ok?" epilogue to it.)
I'd argue this is a related problem but a different one. The diff I
sent serializes cloning/destroying pseudo-interfaces. It has value on
its own because *all* if_clone_*() operations are now serialized.
Now you correctly points out that it doesn't fix all the races in the
ioctl path. That's a fact. However without the informations of the
crashes it is hard to reason about the uncounted reference returned by
ifunit().
Taking a rwlock around all ioctl(2) can have effects that are hard to
debug.
> > 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);
> > }
> >
> >
>
