From: Florian Obser <florian@openbsd.org>
Subject: Re: use after free in nd6 dad
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Sat, 13 Sep 2025 17:03:06 +0200

OK florian

On 2025-09-11 21:20 +02, Alexander Bluhm <bluhm@openbsd.org> wrote:
> 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;
>

-- 
In my defence, I have been left unsupervised.