We have this sequence in bridge(4) ioctl(2) path:

	ifs = ifunit(req->ifbr_ifsname);
		if (ifs == NULL) {
			error = ENOENT;
			break;
		}
	if (ifs->if_bridgeidx != ifp->if_index) {
		error = ESRCH;
		break;
	}
	bif = bridge_getbif(ifs);

This sequence repeats 8 times. Also we don't check value returned by
bridge_getbig() before use. Newly introduced bridge_getbig() function
replaces this sequence. This not only reduces duplicated code but also
makes `bif' dereference safe.

ok bluhm@

diff --git a/sys/net/bridgectl.c b/sys/net/bridgectl.c
index 491770f..6af65b1 100644 (file)
--- a/sys/net/bridgectl.c
+++ b/sys/net/bridgectl.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: bridgectl.c,v 1.21 2020/06/24 22:03:42 cheloha Exp $  */
+/*     $OpenBSD: bridgectl.c,v 1.22 2021/01/25 19:47:16 mvs Exp $      */
 
 /*
  * Copyright (c) 1999, 2000 Jason L. Wright (jason@thought.net)
@@ -120,22 +120,15 @@ bridgectl_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
                bparam->ifbrp_ctime = sc->sc_brttimeout;
                break;
        case SIOCBRDGARL:
-               ifs = ifunit(brlreq->ifbr_ifsname);
-               if (ifs == NULL) {
-                       error = ENOENT;
-                       break;
-               }
-               if (ifs->if_bridgeidx != ifp->if_index) {
-                       error = ESRCH;
-                       break;
-               }
                if ((brlreq->ifbr_action != BRL_ACTION_BLOCK &&
                    brlreq->ifbr_action != BRL_ACTION_PASS) ||
                    (brlreq->ifbr_flags & (BRL_FLAG_IN|BRL_FLAG_OUT)) == 0) {
                        error = EINVAL;
                        break;
                }
-               bif = bridge_getbif(ifs);
+               error = bridge_findbif(sc, brlreq->ifbr_ifsname, &bif);
+               if (error != 0)
+                       break;
                if (brlreq->ifbr_flags & BRL_FLAG_IN) {
                        error = bridge_addrule(bif, brlreq, 0);
                        if (error)
@@ -148,29 +141,15 @@ bridgectl_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
                }
                break;
        case SIOCBRDGFRL:
-               ifs = ifunit(brlreq->ifbr_ifsname);
-               if (ifs == NULL) {
-                       error = ENOENT;
-                       break;
-               }
-               if (ifs->if_bridgeidx != ifp->if_index) {
-                       error = ESRCH;
+               error = bridge_findbif(sc, brlreq->ifbr_ifsname, &bif);
+               if (error != 0)
                        break;
-               }
-               bif = bridge_getbif(ifs);
                bridge_flushrule(bif);
                break;
        case SIOCBRDGGRL:
-               ifs = ifunit(bc->ifbrl_ifsname);
-               if (ifs == NULL) {
-                       error = ENOENT;
+               error = bridge_findbif(sc, bc->ifbrl_ifsname, &bif);
+               if (error != 0)
                        break;
-               }
-               if (ifs->if_bridgeidx != ifp->if_index) {
-                       error = ESRCH;
-                       break;
-               }
-               bif = bridge_getbif(ifs);
                error = bridge_brlconf(bif, bc);
                break;
        default:

diff --git a/sys/net/bridgestp.c b/sys/net/bridgestp.c
index 84feabd..6366841 100644 (file)
--- a/sys/net/bridgestp.c
+++ b/sys/net/bridgestp.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: bridgestp.c,v 1.75 2020/07/30 11:32:06 mvs Exp $      */
+/*     $OpenBSD: bridgestp.c,v 1.76 2021/01/25 19:47:16 mvs Exp $      */
 
 /*
  * Copyright (c) 2000 Jason L. Wright (jason@thought.net)
@@ -2136,23 +2136,15 @@ bstp_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
        struct ifbrparam *ifbp = (struct ifbrparam *)data;
        struct ifbreq *ifbr = (struct ifbreq *)data;
        struct bridge_iflist *bif;
-       struct ifnet *ifs;
        struct bstp_port *bp;
        int r = 0, err = 0, val;
 
        switch (cmd) {
        case SIOCBRDGSIFPRIO:
        case SIOCBRDGSIFCOST:
-               ifs = ifunit(ifbr->ifbr_ifsname);
-               if (ifs == NULL) {
-                       err = ENOENT;
+               err = bridge_findbif(sc, ifbr->ifbr_ifsname, &bif);
+               if (err != 0)
                        break;
-               }
-               if (ifs->if_bridgeidx != ifp->if_index) {
-                       err = ESRCH;
-                       break;
-               }
-               bif = bridge_getbif(ifs);
                if ((bif->bif_flags & IFBIF_STP) == 0) {
                        err = EINVAL;
                        break;

diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
index 337e133..769b384 100644 (file)
--- a/sys/net/if_bridge.c
+++ b/sys/net/if_bridge.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_bridge.c,v 1.347 2021/01/08 23:31:53 dlg Exp $     */
+/*     $OpenBSD: if_bridge.c,v 1.348 2021/01/25 19:47:16 mvs Exp $     */
 
 /*
  * Copyright (c) 1999, 2000 Jason L. Wright (jason@thought.net)
@@ -336,16 +336,9 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
        case SIOCBRDGDEL:
                if ((error = suser(curproc)) != 0)
                        break;
-               ifs = ifunit(req->ifbr_ifsname);
-               if (ifs == NULL) {
-                       error = ENOENT;
-                       break;
-               }
-               if (ifs->if_bridgeidx != ifp->if_index) {
-                       error = ESRCH;
+               error = bridge_findbif(sc, req->ifbr_ifsname, &bif);
+               if (error != 0)
                        break;
-               }
-               bif = bridge_getbif(ifs);
                bridge_ifremove(bif);
                break;
        case SIOCBRDGIFS:
@@ -411,16 +404,9 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
                bridge_spanremove(bif);
                break;
        case SIOCBRDGGIFFLGS:
-               ifs = ifunit(req->ifbr_ifsname);
-               if (ifs == NULL) {
-                       error = ENOENT;
-                       break;
-               }
-               if (ifs->if_bridgeidx != ifp->if_index) {
-                       error = ESRCH;
+               error = bridge_findbif(sc, req->ifbr_ifsname, &bif);
+               if (error != 0)
                        break;
-               }
-               bif = bridge_getbif(ifs);
                req->ifbr_ifsflags = bif->bif_flags;
                req->ifbr_portno = bif->ifp->if_index & 0xfff;
                req->ifbr_protected = bif->bif_protected;
@@ -428,22 +414,15 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
                        bridge_bifgetstp(sc, bif, req);
                break;
        case SIOCBRDGSIFFLGS:
-               if ((error = suser(curproc)) != 0)
-                       break;
-               ifs = ifunit(req->ifbr_ifsname);
-               if (ifs == NULL) {
-                       error = ENOENT;
-                       break;
-               }
-               if (ifs->if_bridgeidx != ifp->if_index) {
-                       error = ESRCH;
-                       break;
-               }
                if (req->ifbr_ifsflags & IFBIF_RO_MASK) {
                        error = EINVAL;
                        break;
                }
-               bif = bridge_getbif(ifs);
+               if ((error = suser(curproc)) != 0)
+                       break;
+               error = bridge_findbif(sc, req->ifbr_ifsname, &bif);
+               if (error != 0)
+                       break;
                if (req->ifbr_ifsflags & IFBIF_STP) {
                        if ((bif->bif_flags & IFBIF_STP) == 0) {
                                /* Enable STP */
@@ -489,16 +468,9 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
                brop->ifbop_last_tc_time.tv_usec = bs->bs_last_tc_time.tv_usec;
                break;
        case SIOCBRDGSIFPROT:
-               ifs = ifunit(req->ifbr_ifsname);
-               if (ifs == NULL) {
-                       error = ENOENT;
-                       break;
-               }
-               if (ifs->if_bridgeidx != ifp->if_index) {
-                       error = ESRCH;
+               error = bridge_findbif(sc, req->ifbr_ifsname, &bif);
+               if (error != 0)
                        break;
-               }
-               bif = bridge_getbif(ifs);
                bif->bif_protected = req->ifbr_protected;
                break;
        case SIOCBRDGRTS:
@@ -703,6 +675,34 @@ done:
        return (error);
 }
 
+int
+bridge_findbif(struct bridge_softc *sc, const char *name,
+    struct bridge_iflist **rbif)
+{
+       struct ifnet *ifp;
+       struct bridge_iflist *bif;
+
+       KERNEL_ASSERT_LOCKED();
+
+       if ((ifp = ifunit(name)) == NULL)
+               return (ENOENT);
+
+       if (ifp->if_bridgeidx != sc->sc_if.if_index)
+               return (ESRCH);
+
+       SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) {
+               if (bif->ifp == ifp)
+                       break;
+       }
+
+       if (bif == NULL)
+               return (ENOENT);
+
+       *rbif = bif;
+       
+       return (0);
+}
+
 struct bridge_iflist *
 bridge_getbif(struct ifnet *ifp)
 {

diff --git a/sys/net/if_bridge.h b/sys/net/if_bridge.h
index 32fb482..0838142 100644 (file)
--- a/sys/net/if_bridge.h
+++ b/sys/net/if_bridge.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_bridge.h,v 1.70 2020/07/30 11:32:06 mvs Exp $      */
+/*     $OpenBSD: if_bridge.h,v 1.71 2021/01/25 19:47:16 mvs Exp $      */
 
 /*
  * Copyright (c) 1999, 2000 Jason L. Wright (jason@thought.net)
@@ -531,6 +531,8 @@ void        bridge_flushrule(struct bridge_iflist *);
 void   bridge_fragment(struct ifnet *, struct ifnet *, struct ether_header *,
     struct mbuf *);
 struct bridge_iflist *bridge_getbif(struct ifnet *);
+int    bridge_findbif(struct bridge_softc *, const char *,
+    struct bridge_iflist **);
 
 #endif /* _KERNEL */
 #endif /* _NET_IF_BRIDGE_H_ */