Diff below is a rework of Eygene's submission to avoid duplicating the logic leading to the re-enqueue of a packet based on a matching MAC address.
The bug first explained by Eygene [0] happens when multiple members of a bridge(4) share the same MAC address. In that particular case the order of the members matter as the first one encounter during the loop will be considered as the "receiving" interface. The idea of the fix is to prefer the physical interface instead, which in this case is referenced by the `ifp' argument of bridge_process(). The diff below does a bit of plumbing to avoid code duplication: - rename the original port member descriptor from `bif' to `bif0' - check for bad source MAC (loop prevention) early - check for wrongly crafted packet before dereferencing `eh' Ok? [0] https://marc.info/?l=openbsd-tech&m=155993043300702&w=2 Index: net/if_bridge.c =================================================================== RCS file: /cvs/src/sys/net/if_bridge.c,v retrieving revision 1.336 diff -u -p -r1.336 if_bridge.c --- net/if_bridge.c 17 Jul 2019 16:46:17 -0000 1.336 +++ net/if_bridge.c 17 Jul 2019 19:34:19 -0000 @@ -931,10 +931,6 @@ bridgeintr_frame(struct ifnet *brifp, st bif = bridge_getbif(src_if); KASSERT(bif != NULL); - if (m->m_pkthdr.len < sizeof(eh)) { - m_freem(m); - return; - } m_copydata(m, 0, ETHER_HDR_LEN, (caddr_t)&eh); dst = (struct ether_addr *)&eh.ether_dhost[0]; src = (struct ether_addr *)&eh.ether_shost[0]; @@ -1115,7 +1111,7 @@ bridge_process(struct ifnet *ifp, struct { struct ifnet *brifp; struct bridge_softc *sc; - struct bridge_iflist *bif, *bif0; + struct bridge_iflist *bif = NULL, *bif0 = NULL; struct ether_header *eh; struct mbuf *mc; #if NBPFILTER > 0 @@ -1128,6 +1124,9 @@ bridge_process(struct ifnet *ifp, struct if ((brifp == NULL) || !ISSET(brifp->if_flags, IFF_RUNNING)) goto reenqueue; + if (m->m_pkthdr.len < sizeof(*eh)) + goto bad; + #if NVLAN > 0 /* * If the underlying interface removed the VLAN header itself, @@ -1146,17 +1145,21 @@ bridge_process(struct ifnet *ifp, struct bpf_mtap_ether(if_bpf, m, BPF_DIRECTION_IN); #endif + eh = mtod(m, struct ether_header *); + sc = brifp->if_softc; SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) { - if (bif->ifp == ifp) - break; + if (bridge_ourether(bif->ifp, eh->ether_shost)) + goto bad; + if (bif->ifp == ifp) { + bif0 = bif; + } } - if (bif == NULL) + if (bif0 == NULL) goto reenqueue; bridge_span(brifp, m); - eh = mtod(m, struct ether_header *); if (ETHER_IS_MULTICAST(eh->ether_dhost)) { /* * Reserved destination MAC addresses (01:80:C2:00:00:0x) @@ -1169,7 +1172,8 @@ bridge_process(struct ifnet *ifp, struct ETHER_ADDR_LEN - 1) == 0) { if (eh->ether_dhost[ETHER_ADDR_LEN - 1] == 0) { /* STP traffic */ - m = bstp_input(sc->sc_stp, bif->bif_stp, eh, m); + m = bstp_input(sc->sc_stp, bif0->bif_stp, eh, + m); if (m == NULL) goto bad; } else if (eh->ether_dhost[ETHER_ADDR_LEN - 1] <= 0xf) @@ -1179,8 +1183,8 @@ bridge_process(struct ifnet *ifp, struct /* * No need to process frames for ifs in the discarding state */ - if ((bif->bif_flags & IFBIF_STP) && - (bif->bif_state == BSTP_IFSTATE_DISCARDING)) + if ((bif0->bif_flags & IFBIF_STP) && + (bif0->bif_state == BSTP_IFSTATE_DISCARDING)) goto reenqueue; mc = m_dup_pkt(m, ETHER_ALIGN, M_NOWAIT); @@ -1197,27 +1201,32 @@ bridge_process(struct ifnet *ifp, struct /* * Unicast, make sure it's not for us. */ - bif0 = bif; - SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) { - if (bridge_ourether(bif->ifp, eh->ether_dhost)) { - if (bif0->bif_flags & IFBIF_LEARNING) - bridge_rtupdate(sc, - (struct ether_addr *)&eh->ether_shost, - ifp, 0, IFBAF_DYNAMIC, m); - if (bridge_filterrule(&bif0->bif_brlin, eh, m) == - BRL_ACTION_BLOCK) { - goto bad; - } - - /* Count for the bridge */ - brifp->if_ipackets++; - brifp->if_ibytes += m->m_pkthdr.len; - - ifp = bif->ifp; - goto reenqueue; + if (bridge_ourether(bif0->ifp, eh->ether_dhost)) { + bif = bif0; + } else { + SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) { + if (bif->ifp == ifp) + continue; + if (bridge_ourether(bif->ifp, eh->ether_dhost)) + break; } - if (bridge_ourether(bif->ifp, eh->ether_shost)) + } + if (bif != NULL) { + if (bif0->bif_flags & IFBIF_LEARNING) + bridge_rtupdate(sc, + (struct ether_addr *)&eh->ether_shost, + ifp, 0, IFBAF_DYNAMIC, m); + if (bridge_filterrule(&bif0->bif_brlin, eh, m) == + BRL_ACTION_BLOCK) { goto bad; + } + + /* Count for the bridge */ + brifp->if_ipackets++; + brifp->if_ibytes += m->m_pkthdr.len; + + ifp = bif->ifp; + goto reenqueue; } bridgeintr_frame(brifp, ifp, m);
