On Mon, Nov 27, 2017 at 10:43:09AM +0100, Martin Pieuchot wrote:
> Here's a diff that includes that and prevent a user-after-free pointed
> out by visa@. We should not try to dereference `rt' if nd6_free() has
> been called.
>
> Hrvoje Popovski confirmed he couldn't reproduce the panic with this
> diff.
>
> ok?
OK bluhm@
> Index: netinet6/nd6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.221
> diff -u -p -r1.221 nd6.c
> --- netinet6/nd6.c 31 Oct 2017 22:05:13 -0000 1.221
> +++ netinet6/nd6.c 27 Nov 2017 09:35:41 -0000
> @@ -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_ifadd
> * 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
> 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
> 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
> }
> 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);
> }
> }
>
> Index: netinet6/nd6.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/nd6.h,v
> retrieving revision 1.73
> diff -u -p -r1.73 nd6.h
> --- netinet6/nd6.h 3 Nov 2017 14:28:57 -0000 1.73
> +++ netinet6/nd6.h 27 Nov 2017 09:29:51 -0000
> @@ -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_o
> 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 *);
