From: Alexander Bluhm <bluhm@openbsd.org>
Subject: use after free in nd6 dad
To: tech@openbsd.org
Date: Thu, 11 Sep 2025 21:20:59 +0200

Hi,

In IPv6 neghbor discovery, the duplicate address detection code
could trigger a use after free.  I have seen a data modifed on
freelist panic.  It looks like the timeout field of struct dadq.

nd6_dad_stop() calls nd6_dad_stoptimer() and nd6_dad_destroy()
without waiting for the timer to run before freeing.  I don't want
to use a barrier in IP input packet path.  Both this code and the
timer runs with exclusive netlock which does not work with barriers.
Refcounting struct dadq feels like overkill.  So I implemented a
reaper on the timeout queue.

While there remove a useless NULL check in nd6_dad_timer().

ok?

bluhm

Index: netinet6/nd6_nbr.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6_nbr.c,v
diff -u -p -r1.162 nd6_nbr.c
--- netinet6/nd6_nbr.c	23 Jul 2025 22:32:49 -0000	1.162
+++ netinet6/nd6_nbr.c	11 Sep 2025 18:49:24 -0000
@@ -73,6 +73,7 @@ struct dadq {
 
 struct dadq *nd6_dad_find(struct ifaddr *);
 void nd6_dad_destroy(struct dadq *);
+void nd6_dad_reaper(void *);
 void nd6_dad_starttimer(struct dadq *);
 void nd6_dad_stoptimer(struct dadq *);
 void nd6_dad_timer(void *);
@@ -994,9 +995,18 @@ void
 nd6_dad_destroy(struct dadq *dp)
 {
 	TAILQ_REMOVE(&dadq, dp, dad_list);
+	ip6_dad_pending--;
+	timeout_set_proc(&dp->dad_timer_ch, nd6_dad_reaper, dp);
+	timeout_add(&dp->dad_timer_ch, 0);
+}
+
+void
+nd6_dad_reaper(void *arg)
+{
+	struct dadq *dp = arg;
+
 	ifafree(dp->dad_ifa);
 	free(dp, M_IP6NDP, sizeof(*dp));
-	ip6_dad_pending--;
 }
 
 void
@@ -1089,9 +1099,9 @@ nd6_dad_stop(struct ifaddr *ifa)
 }
 
 void
-nd6_dad_timer(void *xifa)
+nd6_dad_timer(void *arg)
 {
-	struct ifaddr *ifa;
+	struct ifaddr *ifa = arg;
 	struct in6_ifaddr *ia6;
 	struct in6_addr daddr6, taddr6;
 	struct ifnet *ifp;
@@ -1100,12 +1110,6 @@ nd6_dad_timer(void *xifa)
 
 	NET_LOCK();
 
-	/* Sanity check */
-	if (xifa == NULL) {
-		log(LOG_ERR, "%s: called with null parameter\n", __func__);
-		goto done;
-	}
-	ifa = xifa;
 	ia6 = ifatoia6(ifa);
 	taddr6 = ia6->ia_addr.sin6_addr;
 	ifp = ifa->ifa_ifp;