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