bridge(4)+pf(4) fix incoming interface

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

bridge(4)+pf(4) fix incoming interface

Martin Pieuchot
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);

Reply | Threaded
Open this post in threaded view
|

Re: bridge(4)+pf(4) fix incoming interface

Alexander Bluhm
On Wed, Jul 17, 2019 at 04:35:22PM -0300, Martin Pieuchot wrote:

> 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?

OK bluhm@

>   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;
> + }

This  { } is not KNF.

Reply | Threaded
Open this post in threaded view
|

Re: bridge(4)+pf(4) fix incoming interface

Eygene Ryabinkin
In reply to this post by Martin Pieuchot
Wed, Jul 17, 2019 at 04:35:22PM -0300, Martin Pieuchot wrote:
> 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?

OK for me, but I have some "plumbing" corrections to the patched
version.  Attached.

Thank you!
--
rea

0001-if_bridge-refactoring.patch (3K) Download Attachment