Fix a route use after free in multicast route. Move the rt_mcast_del()

out of the rtable_walk().  This avoids recursion to prevent stack
overflow.  Also it allows freeing the route outside of the walk.
Now mrt_mcast_del() frees the route only when it is deleted from
the routing table.  If that fails, it must not be freed.  After the
route is returned by mfc_find(), it is reference counted.  Then we
need a rtfree(), but not in the other caes.
Move rt_timer_remove_all() into rt_mcast_del().
OK mpi@

1 files changed, 36 insertions, 33 deletions

diff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mroute.c
index 70af2892ccd..feb43cd2a65 100644
--- a/sys/netinet/ip_mroute.c
+++ b/sys/netinet/ip_mroute.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_mroute.c,v 1.127 2019/06/21 17:11:42 mpi Exp $	*/
+/*	$OpenBSD: ip_mroute.c,v 1.128 2019/09/02 13:12:09 bluhm Exp $	*/
 /*	$NetBSD: ip_mroute.c,v 1.85 2004/04/26 01:31:57 matt Exp $	*/
 
 /*
@@ -127,7 +127,7 @@ int ip_mdq(struct mbuf *, struct ifnet *, struct rtentry *);
 struct ifnet *if_lookupbyvif(vifi_t, unsigned int);
 struct rtentry *rt_mcast_add(struct ifnet *, struct sockaddr *,
     struct sockaddr *);
-int rt_mcast_del(struct rtentry *, unsigned int);
+void mrt_mcast_del(struct rtentry *, unsigned int);
 
 /*
  * Kernel multicast routing API capabilities and setup.
@@ -532,10 +532,7 @@ mrouter_rtwalk_delete(struct rtentry *rt, void *arg, unsigned int rtableid)
 	    (RTF_HOST | RTF_MULTICAST))
 		return (0);
 
-	/* Remove all timers related to this route. */
-	rt_timer_remove_all(rt);
-	rt_mcast_del(rt, rtableid);
-	return (0);
+	return EEXIST;
 }
 
 /*
@@ -547,12 +544,24 @@ ip_mrouter_done(struct socket *so)
 	struct inpcb *inp = sotoinpcb(so);
 	struct ifnet *ifp;
 	unsigned int rtableid = inp->inp_rtableid;
+	int error;
 
 	NET_ASSERT_LOCKED();
 
 	/* Delete all remaining installed multicast routes. */
-	rtable_walk(rtableid, AF_INET, NULL, mrouter_rtwalk_delete, NULL);
+	do {
+		struct rtentry *rt = NULL;
+
+		error = rtable_walk(rtableid, AF_INET, &rt,
+		    mrouter_rtwalk_delete, NULL);
+		if (rt != NULL && error == EEXIST) {
+			mrt_mcast_del(rt, rtableid);
+			error = EAGAIN;
+		}
+		rtfree(rt);
+	} while (error == EAGAIN);
 
+	/* Unregister all interfaces in the domain. */
 	TAILQ_FOREACH(ifp, &ifnet, if_list) {
 		if (ifp->if_rdomain != rtableid)
 			continue;
@@ -792,9 +801,7 @@ mfc_expire_route(struct rtentry *rt, struct rttimer *rtt)
 		return;
 	}
 
-	/* Remove all timers related to this route. */
-	rt_timer_remove_all(rt);
-	rt_mcast_del(rt, rtableid);
+	mrt_mcast_del(rt, rtableid);
 }
 
 int
@@ -817,7 +824,8 @@ mfc_add_route(struct ifnet *ifp, struct sockaddr *origin,
 		    satosin(origin)->sin_addr.s_addr,
 		    satosin(group)->sin_addr.s_addr,
 		    mfccp->mfcc_parent, ifp->if_xname);
-		rt_mcast_del(rt, rtableid);
+		mrt_mcast_del(rt, rtableid);
+		rtfree(rt);
 		return (ENOMEM);
 	}
 
@@ -885,8 +893,8 @@ update_mfc_params(struct mfcctl2 *mfccp, int wait, unsigned int rtableid)
 
 			DPRINTF("del route (group %#08X) for vif %d (%s)",
 			    mfccp->mfcc_mcastgrp.s_addr, i, ifp->if_xname);
-			rt_timer_remove_all(rt);
-			rt_mcast_del(rt, rtableid);
+			mrt_mcast_del(rt, rtableid);
+			rtfree(rt);
 			continue;
 		}
 
@@ -1033,9 +1041,8 @@ del_mfc(struct socket *so, struct mbuf *m)
 
 	while ((rt = mfc_find(NULL, &mfcctl2.mfcc_origin,
 	    &mfcctl2.mfcc_mcastgrp, rtableid)) != NULL) {
-		/* Remove all timers related to this route. */
-		rt_timer_remove_all(rt);
-		rt_mcast_del(rt, rtableid);
+		mrt_mcast_del(rt, rtableid);
+		rtfree(rt);
 	}
 
 	return (0);
@@ -1325,30 +1332,26 @@ rt_mcast_add(struct ifnet *ifp, struct sockaddr *origin, struct sockaddr *group)
 	return (mfc_find(ifp, NULL, &satosin(group)->sin_addr, rtableid));
 }
 
-int
-rt_mcast_del(struct rtentry *rt, unsigned int rtableid)
+void
+mrt_mcast_del(struct rtentry *rt, unsigned int rtableid)
 {
 	struct ifnet		*ifp;
-	int			 rv;
+	int			 error;
+
+	/* Remove all timers related to this route. */
+	rt_timer_remove_all(rt);
 
 	free(rt->rt_llinfo, M_MRTABLE, sizeof(struct mfc));
 	rt->rt_llinfo = NULL;
 
-	if ((ifp = if_get(rt->rt_ifidx)) == NULL) {
-		DPRINTF("if_get(%d) failed", rt->rt_ifidx);
-		rtfree(rt);
-		return (ENOENT);
-	}
-
-	rv = rtdeletemsg(rt, ifp, rtableid);
+	ifp = if_get(rt->rt_ifidx);
+	if (ifp == NULL)
+		return;
+	error = rtdeletemsg(rt, ifp, rtableid);
 	if_put(ifp);
-	if (rv != 0) {
-		DPRINTF("rtdeletemsg failed (%d)", rv);
-		rtfree(rt);
-		return (rv);
-	}
 
-	mrt_count[rtableid]--;
+	if (error)
+		DPRINTF("delete route error %d\n", error);
 
-	return (0);
+	mrt_count[rtableid]--;
 }