Use a single timer for all ND6 entries.

This prevents a use-after-free reported by Hrvoje Popovski where the
timeout function was already sleeping on the NET_LOCK() when ifconfig(8)
removed the enry from the table.

By iterating on a global list in the timeout routine we ensure that the
items are still valid when we process them.  This also reduce differences
with ARP.

ok bluhm@, visa@

2 files changed, 68 insertions, 46 deletions

diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c
index f87036bd428..8b0b3d88215 100644
--- a/sys/netinet6/nd6.c
+++ b/sys/netinet6/nd6.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: nd6.c,v 1.221 2017/10/31 22:05:13 sashan Exp $	*/
+/*	$OpenBSD: nd6.c,v 1.222 2017/11/27 15:41:30 mpi Exp $	*/
 /*	$KAME: nd6.c,v 1.280 2002/06/08 19:52:07 itojun Exp $	*/
 
 /*
@@ -67,7 +67,8 @@
 #define ND6_RECALC_REACHTM_INTERVAL (60 * 120) /* 2 hours */
 
 /* timer values */
-time_t	nd6_expire_time	= -1;	/* at which time_uptime nd6_expire runs */
+int	nd6_timer_next	= -1;	/* at which time_uptime nd6_timer runs */
+time_t	nd6_expire_next	= -1;	/* at which time_uptime nd6_expire runs */
 int	nd6_delay	= 5;	/* delay first probe time 5 second */
 int	nd6_umaxtries	= 3;	/* maximum unicast query */
 int	nd6_mmaxtries	= 3;	/* maximum multicast query */
@@ -90,13 +91,15 @@ int	nd6_inuse, nd6_allocated;
 
 int nd6_recalc_reachtm_interval = ND6_RECALC_REACHTM_INTERVAL;
 
+void nd6_timer(void *);
 void nd6_slowtimo(void *);
 void nd6_expire(void *);
 void nd6_expire_timer(void *);
 void nd6_invalidate(struct rtentry *);
 void nd6_free(struct rtentry *);
-void nd6_llinfo_timer(void *);
+int nd6_llinfo_timer(struct rtentry *);
 
+struct timeout nd6_timer_to;
 struct timeout nd6_slowtimo_ch;
 struct timeout nd6_expire_timeout;
 struct task nd6_expire_task;
@@ -120,6 +123,7 @@ nd6_init(void)
 	nd6_init_done = 1;
 
 	/* start timer */
+	timeout_set_proc(&nd6_timer_to, nd6_timer, &nd6_timer_to);
 	timeout_set_proc(&nd6_slowtimo_ch, nd6_slowtimo, NULL);
 	timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
 	timeout_set(&nd6_expire_timeout, nd6_expire_timer, NULL);
@@ -297,45 +301,66 @@ skip1:
  * ND6 timer routine to handle ND6 entries
  */
 void
-nd6_llinfo_settimer(struct llinfo_nd6 *ln, int secs)
+nd6_llinfo_settimer(struct llinfo_nd6 *ln, unsigned int secs)
 {
-	if (secs < 0) {
-		ln->ln_rt->rt_expire = 0;
-		timeout_del(&ln->ln_timer_ch);
-	} else {
-		ln->ln_rt->rt_expire = time_uptime + secs;
-		timeout_add_sec(&ln->ln_timer_ch, secs);
+	time_t expire = time_uptime + secs;
+
+	NET_ASSERT_LOCKED();
+
+	ln->ln_rt->rt_expire = expire;
+	if (!timeout_pending(&nd6_timer_to) || expire < nd6_timer_next) {
+		nd6_timer_next = expire;
+		timeout_add_sec(&nd6_timer_to, secs);
 	}
 }
 
 void
-nd6_llinfo_timer(void *arg)
+nd6_timer(void *arg)
 {
-	struct llinfo_nd6 *ln;
-	struct rtentry *rt;
-	struct sockaddr_in6 *dst;
-	struct ifnet *ifp;
-	struct nd_ifinfo *ndi = NULL;
+	struct llinfo_nd6 *ln, *nln;
+	time_t expire = time_uptime + nd6_gctimer;
+	int secs;
 
 	NET_LOCK();
+	TAILQ_FOREACH_SAFE(ln, &nd6_list, ln_list, nln) {
+		struct rtentry *rt = ln->ln_rt;
 
-	ln = (struct llinfo_nd6 *)arg;
+		if (rt->rt_expire && rt->rt_expire <= time_uptime)
+			if (nd6_llinfo_timer(rt))
+				continue;
 
-	if ((rt = ln->ln_rt) == NULL)
-		panic("ln->ln_rt == NULL");
-	if ((ifp = if_get(rt->rt_ifidx)) == NULL) {
-		NET_UNLOCK();
-		return;
+		if (rt->rt_expire && rt->rt_expire < expire)
+			expire = rt->rt_expire;
 	}
-	ndi = ND_IFINFO(ifp);
-	dst = satosin6(rt_key(rt));
 
-	/* sanity check */
-	if (rt->rt_llinfo != NULL && (struct llinfo_nd6 *)rt->rt_llinfo != ln)
-		panic("rt_llinfo(%p) is not equal to ln(%p)",
-		      rt->rt_llinfo, ln);
-	if (!dst)
-		panic("dst=0 in nd6_timer(ln=%p)", ln);
+	secs = expire - time_uptime;
+	if (secs < 0)
+		secs = 0;
+	if (!TAILQ_EMPTY(&nd6_list))
+		timeout_add_sec(&nd6_timer_to, secs);
+
+	NET_UNLOCK();
+}
+
+/*
+ * ND timer state handling.
+ *
+ * Returns 1 if `rt' should no longer be used, 0 otherwise.
+ */
+int
+nd6_llinfo_timer(struct rtentry *rt)
+{
+	struct llinfo_nd6 *ln = (struct llinfo_nd6 *)rt->rt_llinfo;
+	struct sockaddr_in6 *dst = satosin6(rt_key(rt));
+	struct ifnet *ifp;
+	struct nd_ifinfo *ndi = NULL;
+
+	NET_ASSERT_LOCKED();
+
+	if ((ifp = if_get(rt->rt_ifidx)) == NULL)
+		return 1;
+
+	ndi = ND_IFINFO(ifp);
 
 	switch (ln->ln_state) {
 	case ND6_LLINFO_INCOMPLETE:
@@ -408,7 +433,8 @@ nd6_llinfo_timer(void *arg)
 	}
 
 	if_put(ifp);
-	NET_UNLOCK();
+
+	return (ln == NULL);
 }
 
 void
@@ -436,16 +462,16 @@ nd6_expire_timer_update(struct in6_ifaddr *ia6)
 	 * Schedule timeout one second later so that either IFA6_IS_INVALID()
 	 * or IFA6_IS_DEPRECATED() is true.
 	 */
-	expire_time++; 
+	expire_time++;
 
-	if (!timeout_pending(&nd6_expire_timeout) || nd6_expire_time >
-	    expire_time) {
+	if (!timeout_pending(&nd6_expire_timeout) ||
+	    nd6_expire_next > expire_time) {
 		secs = expire_time - time_uptime;
-		if ( secs < 0)
+		if (secs < 0)
 			secs = 0;
 
 		timeout_add_sec(&nd6_expire_timeout, secs);
-		nd6_expire_time = expire_time;
+		nd6_expire_next = expire_time;
 	}
 }
 
@@ -852,7 +878,6 @@ nd6_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt)
 		nd6_inuse++;
 		nd6_allocated++;
 		ln->ln_rt = rt;
-		timeout_set_proc(&ln->ln_timer_ch, nd6_llinfo_timer, ln);
 		/* this is required for "ndp" command. - shin */
 		if (req == RTM_ADD) {
 		        /*
@@ -913,14 +938,14 @@ nd6_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt)
 		ifa = &in6ifa_ifpwithaddr(ifp,
 		    &satosin6(rt_key(rt))->sin6_addr)->ia_ifa;
 		if (ifa) {
-			nd6_llinfo_settimer(ln, -1);
 			ln->ln_state = ND6_LLINFO_REACHABLE;
 			ln->ln_byhint = 0;
+			rt->rt_expire = 0;
 			KASSERT(ifa == rt->rt_ifa);
 		} else if (rt->rt_flags & RTF_ANNOUNCE) {
-			nd6_llinfo_settimer(ln, -1);
 			ln->ln_state = ND6_LLINFO_REACHABLE;
 			ln->ln_byhint = 0;
+			rt->rt_expire = 0;
 
 			/* join solicited node multicast for proxy ND */
 			if (ifp->if_flags & IFF_MULTICAST) {
@@ -968,7 +993,7 @@ nd6_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt)
 		}
 		nd6_inuse--;
 		TAILQ_REMOVE(&nd6_list, ln, ln_list);
-		nd6_llinfo_settimer(ln, -1);
+		rt->rt_expire = 0;
 		rt->rt_llinfo = NULL;
 		rt->rt_flags &= ~RTF_LLINFO;
 		m_freem(ln->ln_hold);
@@ -1195,7 +1220,7 @@ fail:
 			}
 		} else if (ln->ln_state == ND6_LLINFO_INCOMPLETE) {
 			/* probe right away */
-			nd6_llinfo_settimer((void *)ln, 0);
+			nd6_llinfo_settimer(ln, 0);
 		}
 	}
 
diff --git a/sys/netinet6/nd6.h b/sys/netinet6/nd6.h
index 06fb3a49927..8a7a0f39ca1 100644
--- a/sys/netinet6/nd6.h
+++ b/sys/netinet6/nd6.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: nd6.h,v 1.73 2017/11/03 14:28:57 florian Exp $	*/
+/*	$OpenBSD: nd6.h,v 1.74 2017/11/27 15:41:30 mpi Exp $	*/
 /*	$KAME: nd6.h,v 1.95 2002/06/08 11:31:06 itojun Exp $	*/
 
 /*
@@ -97,7 +97,6 @@ struct	in6_ndifreq {
 #ifdef _KERNEL
 
 #include <sys/queue.h>
-#include <sys/timeout.h>
 
 #define ND_IFINFO(ifp) \
 	(((struct in6_ifextra *)(ifp)->if_afdata[AF_INET6])->nd_ifinfo)
@@ -113,8 +112,6 @@ struct	llinfo_nd6 {
 	int	ln_byhint;	/* # of times we made it reachable by UL hint */
 	short	ln_state;	/* reachability state */
 	short	ln_router;	/* 2^0: ND6 router bit */
-
-	struct	timeout ln_timer_ch;
 };
 
 #define ND6_IS_LLINFO_PROBREACH(n) ((n)->ln_state > ND6_LLINFO_INCOMPLETE)
@@ -173,7 +170,7 @@ struct nd_opt_hdr *nd6_option(union nd_opts *);
 int nd6_options(union nd_opts *);
 struct	rtentry *nd6_lookup(struct in6_addr *, int, struct ifnet *, u_int);
 void nd6_setmtu(struct ifnet *);
-void nd6_llinfo_settimer(struct llinfo_nd6 *, int);
+void nd6_llinfo_settimer(struct llinfo_nd6 *, unsigned int);
 void nd6_purge(struct ifnet *);
 void nd6_nud_hint(struct rtentry *);
 void nd6_rtrequest(struct ifnet *, int, struct rtentry *);