From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: route delete ifp NULL check
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Mon, 4 Aug 2025 13:30:42 +0200

On Mon, Aug 04, 2025 at 05:46:44AM +0300, Vitaliy Makkoveev wrote:
> On Sun, Aug 03, 2025 at 02:26:20PM +0200, Alexander Bluhm wrote:
> > Hi,
> > 
> > Coverity complains that we check ifp NULL pointer in rtrequest_delete(),
> > but later on we dereference it unconditinally.
> > 
> > The only caller that might have a NULL in ifp is nd6_free().  But
> > there we also dereference it later.  I think the exclusive net lock
> > protects us there, but I want to assure it with a kassert.
> > 
> > When we unlock ND6, we have to reconsider this.
> > 
> > ok?
> > 
> 
> Exclusive netlock protects us from `rt_ifidx' change, but not from
> the interface detach from if_idxmap. So hypothetically `ifp' could be
> NULL during race condition. However, seems the nd6_free() callers own
> the `ifp' related to this `rt'. Could we pass them to the nd6_free()
> together?

Good idea, diff below.

My only concern was that sdl->sdl_index is always the same as
rt->rt_ifidx in nd6_purge().  I think we can assume that.

ok?

bluhm

Index: net/route.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
diff -u -p -r1.447 route.c
--- net/route.c	14 Jul 2025 08:48:51 -0000	1.447
+++ net/route.c	4 Aug 2025 10:19:18 -0000
@@ -907,7 +907,7 @@ rtrequest_delete(struct rt_addrinfo *inf
 		return (ESRCH);
 
 	/* Make sure that's the route the caller want to delete. */
-	if (ifp != NULL && ifp->if_index != rt->rt_ifidx) {
+	if (ifp->if_index != rt->rt_ifidx) {
 		rtfree(rt);
 		return (ESRCH);
 	}
Index: netinet6/nd6.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v
diff -u -p -r1.296 nd6.c
--- netinet6/nd6.c	3 Aug 2025 11:08:40 -0000	1.296
+++ netinet6/nd6.c	4 Aug 2025 11:18:49 -0000
@@ -99,7 +99,7 @@ void nd6_slowtimo(void *);
 void nd6_expire(void *);
 void nd6_expire_timer(void *);
 void nd6_invalidate(struct rtentry *);
-void nd6_free(struct rtentry *, int);
+void nd6_free(struct rtentry *, struct ifnet *ifp, int);
 int nd6_llinfo_timer(struct rtentry *, int);
 
 struct timeout nd6_timer_to;
@@ -329,7 +329,7 @@ nd6_llinfo_timer(struct rtentry *rt, int
 			} else
 				atomic_sub_int(&ln_hold_total, len);
 
-			nd6_free(rt, i_am_router);
+			nd6_free(rt, ifp, i_am_router);
 			ln = NULL;
 		}
 		break;
@@ -345,7 +345,7 @@ nd6_llinfo_timer(struct rtentry *rt, int
 	case ND6_LLINFO_PURGE:
 		/* Garbage Collection(RFC 2461 5.3) */
 		if (!ND6_LLINFO_PERMANENT(ln)) {
-			nd6_free(rt, i_am_router);
+			nd6_free(rt, ifp, i_am_router);
 			ln = NULL;
 		}
 		break;
@@ -366,7 +366,7 @@ nd6_llinfo_timer(struct rtentry *rt, int
 			nd6_ns_output(ifp, &dst->sin6_addr, &dst->sin6_addr,
 			    &ln->ln_saddr6, 0);
 		} else {
-			nd6_free(rt, i_am_router);
+			nd6_free(rt, ifp, i_am_router);
 			ln = NULL;
 		}
 		break;
@@ -476,7 +476,7 @@ nd6_purge(struct ifnet *ifp)
 		    rt->rt_gateway->sa_family == AF_LINK) {
 			sdl = satosdl(rt->rt_gateway);
 			if (sdl->sdl_index == ifp->if_index)
-				nd6_free(rt, i_am_router);
+				nd6_free(rt, ifp, i_am_router);
 		}
 	}
 }
@@ -638,16 +638,13 @@ nd6_invalidate(struct rtentry *rt)
  * Free an nd6 llinfo entry.
  */
 void
-nd6_free(struct rtentry *rt, int i_am_router)
+nd6_free(struct rtentry *rt, struct ifnet *ifp, int i_am_router)
 {
 	struct llinfo_nd6 *ln = (struct llinfo_nd6 *)rt->rt_llinfo;
 	struct in6_addr in6 = satosin6(rt_key(rt))->sin6_addr;
-	struct ifnet *ifp;
 
 	NET_ASSERT_LOCKED_EXCLUSIVE();
 
-	ifp = if_get(rt->rt_ifidx);
-
 	if (!i_am_router) {
 		if (ln->ln_router) {
 			/*
@@ -669,8 +666,6 @@ nd6_free(struct rtentry *rt, int i_am_ro
 	 */
 	if (!ISSET(rt->rt_flags, RTF_STATIC|RTF_CACHED))
 		rtdeletemsg(rt, ifp, ifp->if_rdomain);
-
-	if_put(ifp);
 }
 
 /*
@@ -1054,7 +1049,7 @@ nd6_cache_lladdr(struct ifnet *ifp, cons
 		return;
 	if ((rt->rt_flags & (RTF_GATEWAY | RTF_LLINFO)) != RTF_LLINFO) {
 fail:
-		nd6_free(rt, i_am_router);
+		nd6_free(rt, ifp, i_am_router);
 		rtfree(rt);
 		return;
 	}