On Tue, Jun 23, 2020 at 01:00:06AM -0600, 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.
>
> 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.
>
> This patch fixes the issue by making if_clone_{create,destroy} exclusive
> with all other ifioctls.
> ---
> sys/net/if.c | 36 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git sys/net/if.c sys/net/if.c
> index fb2f86f4a7c..9eedea83d32 100644
> --- sys/net/if.c
> +++ sys/net/if.c
> @@ -246,6 +246,11 @@ struct task if_input_task_locked =
> TASK_INITIALIZER(if_netisr, NULL);
> */
> struct rwlock netlock = RWLOCK_INITIALIZER("netlock");
>
> +/*
> + * Protect modifications to objects owned by ifnet's if_list
> + */
> +struct rwlock iflist_obj_lock = RWLOCK_INITIALIZER("iflist_obj_lock");
> +
> /*
> * Network interface utility routines.
> */
> @@ -1905,7 +1910,7 @@ if_setrdomain(struct ifnet *ifp, int rdomain)
> * Interface ioctls.
> */
> int
> -ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> +ifioctl_unlocked(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> {
> struct ifnet *ifp;
> struct ifreq *ifr = (struct ifreq *)data;
> @@ -2432,6 +2437,35 @@ ifioctl_get(u_long cmd, caddr_t data)
> return (error);
> }
>
> +int
> +ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> +{
> + int ret;
> +
> + switch (cmd) {
> + case SIOCIFCREATE:
> + case SIOCIFDESTROY:
> + rw_enter_write(&iflist_obj_lock);
> + break;
> + default:
> + rw_enter_read(&iflist_obj_lock);
> + }
> +
> + ret = ifioctl_unlocked(so, cmd, data, p);
> +
> + switch (cmd) {
> + case SIOCIFCREATE:
> + case SIOCIFDESTROY:
> + rw_exit_write(&iflist_obj_lock);
> + break;
> + default:
> + rw_exit_read(&iflist_obj_lock);
> + }
> +
> + return (ret);
> +}
> +
> +
> static int
> if_sffpage_check(const caddr_t data)
> {
> --
> 2.27.0
>
As mpi@ pointed you can sleep witchin ifioctl(). In most cases you lock
`iflist_obj_lock' for read and you didn't catch deadlock. But you also
can sleep witchin if_clone_create() and if_clone_create() while you lock
`iflist_obj_lock' for write.
As I see the races are:
1. We sleep in if_clone_create() and we don't mark allocated `unit' as
busy. So we can create multiple `ifp's with identical names and break
ifunit().
2. We sleep in if_clone_destroy() and we don't mark `ifp' as dying. So
we can grab this `ifp' by concurent thread.
3. We don't protect `ifp' being used in other cases so we can destroy
it.
Diff below fixes this issues. It's not for commit and I _know_ it's very
ugly. I just want to show the direction to fix this issus as I see it
and it's quickest way.
1. if_clone_create(). We reserve allocated `unit' unit so we can't do
multiple insertion of `ifp' with idential names.
2. if_clone_destroy(). We mark `ifp' as dying. Also we release `unit'
after we do `ifc_destroy' We can't destroy `ifp' more than once.
3. ifunit() will not receive `ifp' marked as dying.
4. We bump `ifp' ref counter in other cases to be sure if_detach() will
wait all concurent threads.
5. I disabled if_deactivate(ifp); in if_bridge. if_detach() will do it
so there is no reason to do it twice. I will do special diff for this.
I have 12:40 now. I run command below since 10:00 and my system is
still stable.
comments?
---- cut begin ----
for i in 1 2 3; do while true; do ifconfig bridge0 create& \
ifconfig bridge0 destroy& done& done
---- cut end ----
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 09:30:46 -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 09:30:46 -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_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 09:30:46 -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
