Introduce new function if_unit(9). This function returns a pointer the

interface descriptor corresponding to the unique name. This descriptor
is guaranteed to be valid until if_put(9) is called on the returned
pointer. if_unit(9) should replace already existent ifunit() which
returns descriptor not safe for dereference when context was switched.
This allow us to avoid some use-after-free issues in ioctl(2) path.
Also this unifies interface descriptor usage.

ok claudio@ sashan@

diff --git a/share/man/man9/if_get.9 b/share/man/man9/if_get.9
index 5174d7c..4138c74 100644 (file)
--- a/share/man/man9/if_get.9
+++ b/share/man/man9/if_get.9
@@ -1,4 +1,4 @@
-.\" $OpenBSD: if_get.9,v 1.2 2015/12/08 18:35:51 jmc Exp $
+.\" $OpenBSD: if_get.9,v 1.3 2021/01/18 09:55:43 mvs Exp $
 .\"
 .\" Copyright (c) 2015 Martin Pieuchot
 .\"
@@ -14,17 +14,20 @@
 .\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 .\"
-.Dd $Mdocdate: December 8 2015 $
+.Dd $Mdocdate: January 18 2021 $
 .Dt IF_GET 9
 .Os
 .Sh NAME
 .Nm if_get ,
+.Nm if_unit ,
 .Nm if_put
 .Nd get an interface pointer from an interface index
 .Sh SYNOPSIS
 .In net/if.h
 .Ft struct ifnet *
 .Fn if_get "unsigned int ifidx"
+.Ft struct ifnet *
+.Fn if_unit "const char *name"
 .Ft void
 .Fn if_put "struct ifnet *ifp"
 .Sh DESCRIPTION
@@ -43,6 +46,15 @@ is never associated with an interface descriptor and can be used to determine if
 an interface index is valid or not.
 .Pp
 The
+.Fn if_unit
+function returns a pointer to the interface descriptor corresponding to the
+unique name
+.Fa name .
+This descriptor is guaranteed to be valid until
+.Fn if_put
+is called on the returned pointer.
+.Pp
+The
 .Fn if_put
 function releases a reference on the interface descriptor pointed by
 .Fa ifp .
@@ -53,6 +65,7 @@ is a
 pointer, no action occurs.
 .Sh CONTEXT
 .Fn if_get ,
+.Fn if_unit
 and
 .Fn if_put
 can be called during autoconf, from process context, or from interrupt context.
@@ -60,3 +73,8 @@ can be called during autoconf, from process context, or from interrupt context.
 .Fn if_get
 returns a pointer to an interface descriptor if the index is valid, otherwise
 .Dv NULL .
+.Pp
+.Fn if_unit
+returns a pointer to an interface descriptor if the interface with present
+name exists, otherwise
+.Dv NULL .

diff --git a/sys/net/if.c b/sys/net/if.c
index 0206594..50d1e1d 100644 (file)
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if.c,v 1.624 2021/01/09 14:55:21 bluhm Exp $  */
+/*     $OpenBSD: if.c,v 1.625 2021/01/18 09:55:43 mvs Exp $    */
 /*     $NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $  */
 
 /*
@@ -134,6 +134,7 @@
 void   if_attachsetup(struct ifnet *);
 void   if_attachdomain(struct ifnet *);
 void   if_attach_common(struct ifnet *);
+void   if_remove(struct ifnet *);
 int    if_createrdomain(int, struct ifnet *);
 int    if_setrdomain(struct ifnet *, int);
 void   if_slowtimo(void *);
@@ -386,9 +387,6 @@ if_idxmap_remove(struct ifnet *ifp)
        srp_update_locked(&if_ifp_gc, &map[index], NULL);
        if_idxmap.count--;
        /* end of if_idxmap modifications */
-
-       /* sleep until the last reference is released */
-       refcnt_finalize(&ifp->if_refcnt, "ifidxrm");
 }
 
 void
@@ -950,6 +948,21 @@ if_hooks_run(struct task_list *hooks)
        mtx_leave(&if_hooks_mtx);
 }
 
+void
+if_remove(struct ifnet *ifp)
+{
+       /* Remove the interface from the list of all interfaces. */
+       NET_LOCK();
+       TAILQ_REMOVE(&ifnet, ifp, if_list);
+       NET_UNLOCK();
+
+       /* Remove the interface from the interface index map. */
+       if_idxmap_remove(ifp);
+
+       /* Sleep until the last reference is released. */
+       refcnt_finalize(&ifp->if_refcnt, "ifrm");
+}
+
 void
 if_deactivate(struct ifnet *ifp)
 {
@@ -995,10 +1008,10 @@ if_detach(struct ifnet *ifp)
        /* Undo pseudo-driver changes. */
        if_deactivate(ifp);
 
-       ifq_clr_oactive(&ifp->if_snd);
-
        /* Other CPUs must not have a reference before we start destroying. */
-       if_idxmap_remove(ifp);
+       if_remove(ifp);
+
+       ifq_clr_oactive(&ifp->if_snd);
 
 #if NBPFILTER > 0
        bpfdetach(ifp);
@@ -1033,9 +1046,6 @@ if_detach(struct ifnet *ifp)
        pfi_detach_ifnet(ifp);
 #endif
 
-       /* Remove the interface from the list of all interfaces.  */
-       TAILQ_REMOVE(&ifnet, ifp, if_list);
-
        while ((ifg = TAILQ_FIRST(&ifp->if_groups)) != NULL)
                if_delgroup(ifp, ifg->ifgl_group->ifg_group);
 
@@ -1141,14 +1151,14 @@ if_clone_create(const char *name, int rdomain)
 
        rw_enter_write(&if_cloners_lock);
 
-       if (ifunit(name) != NULL) {
+       if ((ifp = if_unit(name)) != NULL) {
                ret = EEXIST;
                goto unlock;
        }
 
        ret = (*ifc->ifc_create)(ifc, unit);
 
-       if (ret != 0 || (ifp = ifunit(name)) == NULL)
+       if (ret != 0 || (ifp = if_unit(name)) == NULL)
                goto unlock;
 
        NET_LOCK();
@@ -1158,6 +1168,7 @@ if_clone_create(const char *name, int rdomain)
        NET_UNLOCK();
 unlock:
        rw_exit_write(&if_cloners_lock);
+       if_put(ifp);
 
        return (ret);
 }
@@ -1181,7 +1192,10 @@ if_clone_destroy(const char *name)
 
        rw_enter_write(&if_cloners_lock);
 
-       ifp = ifunit(name);
+       TAILQ_FOREACH(ifp, &ifnet, if_list) {
+               if (strcmp(ifp->if_xname, name) == 0)
+                       break;
+       }
        if (ifp == NULL) {
                rw_exit_write(&if_cloners_lock);
                return (ENXIO);
@@ -1640,7 +1654,7 @@ if_watchdog_task(void *xifidx)
 }
 
 /*
- * Map interface name to interface structure pointer.
+ * Map interface name to interface structure pointer (legacy).
  */
 struct ifnet *
 ifunit(const char *name)
@@ -1656,6 +1670,20 @@ ifunit(const char *name)
        return (NULL);
 }
 
+/*
+ * Map interface name to interface structure pointer.
+ */
+struct ifnet *
+if_unit(const char *name)
+{
+       struct ifnet *ifp;
+
+       if ((ifp = ifunit(name)) != NULL)
+               if_ref(ifp);
+
+       return (ifp);
+}
+
 /*
  * Map interface index to interface structure pointer.
  */
@@ -1727,13 +1755,16 @@ if_createrdomain(int rdomain, struct ifnet *ifp)
        /* Create rdomain including its loopback if with unit == rdomain */
        snprintf(loifname, sizeof(loifname), "lo%u", unit);
        error = if_clone_create(loifname, 0);
-       if ((loifp = ifunit(loifname)) == NULL)
+       if ((loifp = if_unit(loifname)) == NULL)
                return (ENXIO);
-       if (error && (ifp != loifp || error != EEXIST))
+       if (error && (ifp != loifp || error != EEXIST)) {
+               if_put(loifp);
                return (error);
+       }
 
        rtable_l2set(rdomain, rdomain, loifp->if_index);
        loifp->if_rdomain = rdomain;
+       if_put(loifp);
 
        return (0);
 }
@@ -1856,7 +1887,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
                return (ifioctl_get(cmd, data));
        }
 
-       ifp = ifunit(ifr->ifr_name);
+       ifp = if_unit(ifr->ifr_name);
        if (ifp == NULL)
                return (ENXIO);
        oif_flags = ifp->if_flags;
@@ -2214,6 +2245,8 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
        if (((oif_flags ^ ifp->if_flags) & IFF_UP) != 0)
                getmicrotime(&ifp->if_lastchange);
 
+       if_put(ifp);
+
        return (error);
 }
 
@@ -2254,7 +2287,7 @@ ifioctl_get(u_long cmd, caddr_t data)
                return (error);
        }
 
-       ifp = ifunit(ifr->ifr_name);
+       ifp = if_unit(ifr->ifr_name);
        if (ifp == NULL)
                return (ENXIO);
 
@@ -2328,6 +2361,8 @@ ifioctl_get(u_long cmd, caddr_t data)
 
        NET_RUNLOCK_IN_IOCTL();
 
+       if_put(ifp);
+
        return (error);
 }
 

diff --git a/sys/net/if.h b/sys/net/if.h
index 8f8892d..48a7417 100644 (file)
--- a/sys/net/if.h
+++ b/sys/net/if.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if.h,v 1.204 2020/09/30 19:22:51 mvs Exp $    */
+/*     $OpenBSD: if.h,v 1.205 2021/01/18 09:55:43 mvs Exp $    */
 /*     $NetBSD: if.h,v 1.23 1996/05/07 02:40:27 thorpej Exp $  */
 
 /*
@@ -546,6 +546,7 @@ int if_addgroup(struct ifnet *, const char *);
 int    if_delgroup(struct ifnet *, const char *);
 void   if_group_routechange(struct sockaddr *, struct sockaddr *);
 struct ifnet *ifunit(const char *);
+struct ifnet *if_unit(const char *);
 struct ifnet *if_get(unsigned int);
 void   if_put(struct ifnet *);
 void   ifnewlladdr(struct ifnet *);