`struct bstp_state' stores pointer to parent `ifnet' as `bs_ifp'.

Replace this pointer by interface index. This allow us to avoid some use
after free issues caused by ifioctl() races.

ok sashan@

4 files changed, 58 insertions, 19 deletions

diff --git a/sys/net/bridgestp.c b/sys/net/bridgestp.c
index 00d3b1b3d1e..84feabda1f7 100644
--- a/sys/net/bridgestp.c
+++ b/sys/net/bridgestp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: bridgestp.c,v 1.74 2020/07/22 20:37:34 mvs Exp $	*/
+/*	$OpenBSD: bridgestp.c,v 1.75 2020/07/30 11:32:06 mvs Exp $	*/
 
 /*
  * Copyright (c) 2000 Jason L. Wright (jason@thought.net)
@@ -284,8 +284,16 @@ int	bstp_same_bridgeid(u_int64_t, u_int64_t);
 void
 bstp_transmit(struct bstp_state *bs, struct bstp_port *bp)
 {
-	if ((bs->bs_ifflags & IFF_RUNNING) == 0 || bp == NULL)
+	struct ifnet *ifp;
+
+	if ((ifp = if_get(bs->bs_ifindex)) == NULL)
+		return;
+
+	if ((ifp->if_flags & IFF_RUNNING) == 0 || bp == NULL) {
+		if_put(ifp);
 		return;
+	}
+	if_put(ifp);
 
 	/*
 	 * a PDU can only be sent if we have tx quota left and the
@@ -1696,12 +1704,17 @@ void
 bstp_tick(void *arg)
 {
 	struct bstp_state *bs = (struct bstp_state *)arg;
+	struct ifnet *ifp;
 	struct bstp_port *bp;
 	int s;
 
+	if ((ifp = if_get(bs->bs_ifindex)) == NULL)
+		return;
+
 	s = splnet();
-	if ((bs->bs_ifflags & IFF_RUNNING) == 0) {
+	if ((ifp->if_flags & IFF_RUNNING) == 0) {
 		splx(s);
+		if_put(ifp);
 		return;
 	}
 
@@ -1738,10 +1751,11 @@ bstp_tick(void *arg)
 			bp->bp_txcount--;
 	}
 
-	if (bs->bs_ifp->if_flags & IFF_RUNNING)
+	if (ifp->if_flags & IFF_RUNNING)
 		timeout_add_sec(&bs->bs_bstptimeout, 1);
 
 	splx(s);
+	if_put(ifp);
 }
 
 void
@@ -1922,14 +1936,13 @@ bstp_initialization(struct bstp_state *bs)
 }
 
 struct bstp_state *
-bstp_create(struct ifnet *ifp)
+bstp_create(void)
 {
 	struct bstp_state *bs;
 
 	bs = malloc(sizeof(*bs), M_DEVBUF, M_WAITOK|M_ZERO);
 	LIST_INIT(&bs->bs_bplist);
 
-	bs->bs_ifp = ifp;
 	bs->bs_bridge_max_age = BSTP_DEFAULT_MAX_AGE;
 	bs->bs_bridge_htime = BSTP_DEFAULT_HELLO_TIME;
 	bs->bs_bridge_fdelay = BSTP_DEFAULT_FORWARD_DELAY;
@@ -1957,6 +1970,21 @@ bstp_destroy(struct bstp_state *bs)
 }
 
 void
+bstp_enable(struct bstp_state *bs, unsigned int ifindex)
+{
+	bs->bs_ifindex = ifindex;
+	bstp_initialization(bs);
+}
+
+void
+bstp_disable(struct bstp_state *bs)
+{
+	if (timeout_initialized(&bs->bs_bstptimeout))
+		timeout_del(&bs->bs_bstptimeout);
+	bs->bs_ifindex = 0;
+}
+
+void
 bstp_stop(struct bstp_state *bs)
 {
 	struct bstp_port *bp;
diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
index bb9098daaea..78cf3f69967 100644
--- a/sys/net/if_bridge.c
+++ b/sys/net/if_bridge.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_bridge.c,v 1.343 2020/07/22 20:37:35 mvs Exp $	*/
+/*	$OpenBSD: if_bridge.c,v 1.344 2020/07/30 11:32:06 mvs Exp $	*/
 
 /*
  * Copyright (c) 1999, 2000 Jason L. Wright (jason@thought.net)
@@ -169,7 +169,7 @@ bridge_clone_create(struct if_clone *ifc, int unit)
 	int i;
 
 	sc = malloc(sizeof(*sc), M_DEVBUF, M_WAITOK|M_ZERO);
-	sc->sc_stp = bstp_create(&sc->sc_if);
+	sc->sc_stp = bstp_create();
 	if (!sc->sc_stp) {
 		free(sc, M_DEVBUF, sizeof *sc);
 		return (ENOMEM);
@@ -738,7 +738,7 @@ bridge_init(struct bridge_softc *sc)
 	if (ISSET(ifp->if_flags, IFF_RUNNING))
 		return;
 
-	bstp_initialization(sc->sc_stp);
+	bstp_enable(sc->sc_stp, ifp->if_index);
 
 	if (sc->sc_brttimeout != 0)
 		timeout_add_sec(&sc->sc_brtimeout, sc->sc_brttimeout);
@@ -759,6 +759,8 @@ bridge_stop(struct bridge_softc *sc)
 
 	CLR(ifp->if_flags, IFF_RUNNING);
 
+	bstp_disable(sc->sc_stp);
+
 	timeout_del_barrier(&sc->sc_brtimeout);
 
 	bridge_rtflush(sc, IFBF_FLUSHDYN);
diff --git a/sys/net/if_bridge.h b/sys/net/if_bridge.h
index 96f853c30f9..32fb48234a6 100644
--- a/sys/net/if_bridge.h
+++ b/sys/net/if_bridge.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_bridge.h,v 1.69 2020/07/29 12:09:31 mvs Exp $	*/
+/*	$OpenBSD: if_bridge.h,v 1.70 2020/07/30 11:32:06 mvs Exp $	*/
 
 /*
  * Copyright (c) 1999, 2000 Jason L. Wright (jason@thought.net)
@@ -384,7 +384,7 @@ struct bstp_port {
  * Software state for each bridge STP.
  */
 struct bstp_state {
-	struct ifnet		*bs_ifp;
+	unsigned int		bs_ifindex;
 	struct bstp_pri_vector	bs_bridge_pv;
 	struct bstp_pri_vector	bs_root_pv;
 	struct bstp_port	*bs_root_port;
@@ -407,7 +407,6 @@ struct bstp_state {
 	struct timeval		bs_last_tc_time;
 	LIST_HEAD(, bstp_port)	bs_bplist;
 };
-#define	bs_ifflags		bs_ifp->if_flags
 
 /*
  * Bridge interface list
@@ -502,7 +501,9 @@ void	bridge_tunneluntag(struct mbuf *);
 void	bridge_copyaddr(struct sockaddr *, struct sockaddr *);
 void	bridge_copytag(struct bridge_tunneltag *, struct bridge_tunneltag *);
 
-struct bstp_state *bstp_create(struct ifnet *);
+struct bstp_state *bstp_create(void);
+void	bstp_enable(struct bstp_state *bs, unsigned int);
+void	bstp_disable(struct bstp_state *bs);
 void	bstp_destroy(struct bstp_state *);
 void	bstp_initialization(struct bstp_state *);
 void	bstp_stop(struct bstp_state *);
diff --git a/sys/net/if_switch.c b/sys/net/if_switch.c
index 87a1e72cfc2..57373a4f88c 100644
--- a/sys/net/if_switch.c
+++ b/sys/net/if_switch.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_switch.c,v 1.36 2020/07/28 09:52:32 mvs Exp $	*/
+/*	$OpenBSD: if_switch.c,v 1.37 2020/07/30 11:32:06 mvs Exp $	*/
 
 /*
  * Copyright (c) 2016 Kazuya GODA <goda@openbsd.org>
@@ -169,7 +169,7 @@ switch_clone_create(struct if_clone *ifc, int unit)
 	TAILQ_INIT(&sc->sc_swpo_list);
 
 	sc->sc_unit = unit;
-	sc->sc_stp = bstp_create(&sc->sc_if);
+	sc->sc_stp = bstp_create();
 	if (!sc->sc_stp) {
 		free(sc, M_DEVBUF, sizeof(*sc));
 		return (ENOMEM);
@@ -444,11 +444,19 @@ switch_ioctl(struct ifnet *ifp, unsigned long cmd, caddr_t data)
 		breq->ifbr_protected = swpo->swpo_protected;
 		break;
 	case SIOCSIFFLAGS:
-		if ((ifp->if_flags & IFF_UP) == IFF_UP)
-			ifp->if_flags |= IFF_RUNNING;
+		if ((ifp->if_flags & IFF_UP) == IFF_UP) {
+			if ((ifp->if_flags & IFF_RUNNING) == 0) {
+				ifp->if_flags |= IFF_RUNNING;
+				bstp_enable(sc->sc_stp, ifp->if_index);
+			}
+		}
 
-		if ((ifp->if_flags & IFF_UP) == 0)
-			ifp->if_flags &= ~IFF_RUNNING;
+		if ((ifp->if_flags & IFF_UP) == 0) {
+			if ((ifp->if_flags & IFF_RUNNING) == IFF_RUNNING) {
+				ifp->if_flags &= ~IFF_RUNNING;
+				bstp_disable(sc->sc_stp);
+			}
+		}
 
 		break;
 	case SIOCBRDGRTS: