From 044aaac6d42ec1f1777a52f2fa98b542d731d25c Mon Sep 17 00:00:00 2001
From: mvs <mvs@openbsd.org>
Date: Mon, 25 Jan 2021 19:47:16 +0000
Subject: [PATCH] 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@
---
 sys/net/bridgectl.c | 37 +++++----------------
 sys/net/bridgestp.c | 14 ++------
 sys/net/if_bridge.c | 80 ++++++++++++++++++++++-----------------------
 sys/net/if_bridge.h |  4 ++-
 4 files changed, 54 insertions(+), 81 deletions(-)

diff --git a/sys/net/bridgectl.c b/sys/net/bridgectl.c
index 491770f9543..6af65b1aeb7 100644
--- 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 84feabda1f7..6366841e1e1 100644
--- 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 337e133661b..769b3845e39 100644
--- 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 32fb48234a6..0838142d5cb 100644
--- 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_ */
-- 
2.20.1