From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: route delete ifp NULL check
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Mon, 4 Aug 2025 05:46:44 +0300

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?

> 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	3 Aug 2025 12:15:08 -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.295 nd6.c
> --- netinet6/nd6.c	3 Aug 2025 04:11:57 -0000	1.295
> +++ netinet6/nd6.c	3 Aug 2025 12:17:29 -0000
> @@ -647,6 +647,7 @@ nd6_free(struct rtentry *rt, int i_am_ro
>  	NET_ASSERT_LOCKED_EXCLUSIVE();
>  
>  	ifp = if_get(rt->rt_ifidx);
> +	KASSERT(ifp != NULL);
>  
>  	if (!i_am_router) {
>  		if (ln->ln_router) {
>