| Message ID | 1401084953-10135-1-git-send-email-makita.toshiaki@lab.ntt.co.jp |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 05/26/2014 02:15 AM, Toshiaki Makita wrote: > br_handle_local_finish() is allowing us to insert an FDB entry with > disallowed vlan. For example, when port 1 and 2 are communicating in > vlan 10, and even if vlan 10 is disallowed on port 3, port 3 can > interfere with their communication by spoofed src mac address with > vlan id 10. > > Note: Even if it is judged that a frame should not be learned, it should > not be dropped because it is destined for not forwarding layer but higher > layer. See IEEE 802.1Q-2011 8.13.10. > > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > --- > net/bridge/br_input.c | 4 ++-- > net/bridge/br_private.h | 7 +++++++ > net/bridge/br_vlan.c | 28 ++++++++++++++++++++++++++++ > 3 files changed, 37 insertions(+), 2 deletions(-) > > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index 7985dea..04d6348 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -147,8 +147,8 @@ static int br_handle_local_finish(struct sk_buff *skb) > struct net_bridge_port *p = br_port_get_rcu(skb->dev); > u16 vid = 0; > > - br_vlan_get_tag(skb, &vid); > - if (p->flags & BR_LEARNING) > + /* check if vlan is allowed, to avoid spoofing */ > + if (p->flags & BR_LEARNING && br_should_learn(p, skb, &vid)) > br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, false); > return 0; /* process further */ > } > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 06811d7..59d3a85 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -581,6 +581,7 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v, > struct sk_buff *skb, u16 *vid); > bool br_allowed_egress(struct net_bridge *br, const struct net_port_vlans *v, > const struct sk_buff *skb); > +bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid); > struct sk_buff *br_handle_vlan(struct net_bridge *br, > const struct net_port_vlans *v, > struct sk_buff *skb); > @@ -648,6 +649,12 @@ static inline bool br_allowed_egress(struct net_bridge *br, > return true; > } > > +static inline bool br_should_learn(struct net_bridge_port *p, > + struct sk_buff *skb, u16 *vid) > +{ > + return true; > +} > + > static inline struct sk_buff *br_handle_vlan(struct net_bridge *br, > const struct net_port_vlans *v, > struct sk_buff *skb) > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > index 4a37161..5fee2fe 100644 > --- a/net/bridge/br_vlan.c > +++ b/net/bridge/br_vlan.c > @@ -241,6 +241,34 @@ bool br_allowed_egress(struct net_bridge *br, > return false; > } > > +/* Called under RCU */ > +bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid) > +{ > + struct net_bridge *br = p->br; > + struct net_port_vlans *v; > + > + if (!br->vlan_enabled) > + return true; > + > + v = rcu_dereference(p->vlan_info); > + if (!v) > + return false; > + > + br_vlan_get_tag(skb, vid); > + if (!*vid) { > + *vid = br_get_pvid(v); > + if (*vid == VLAN_N_VID) > + return false; > + > + return true; > + } > + > + if (test_bit(*vid, v->vlan_bitmap)) > + return true; > + > + return false; > +} > + > /* Must be protected by RTNL. > * Must be called with vid in range from 1 to 4094 inclusive. > */ > This is very similar to br_allow_ingress(), so may be you can re-factor so that we only have 1 such function... -vlad -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-05-28 at 10:02 -0400, Vlad Yasevich wrote: ... > > This is very similar to br_allow_ingress(), so may be you can > re-factor so that we only have 1 such function... Yes.. indeed, this looks redundant. What bothered me is br_allowed_ingress() modifies or drops frames but I'm not expecting it here. Maybe we can hand a hint to br_allowed_ingress so that it doesn't modify the skb? Thanks, Toshiaki Makita -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> Date: Mon, 26 May 2014 15:15:53 +0900 > br_handle_local_finish() is allowing us to insert an FDB entry with > disallowed vlan. For example, when port 1 and 2 are communicating in > vlan 10, and even if vlan 10 is disallowed on port 3, port 3 can > interfere with their communication by spoofed src mac address with > vlan id 10. > > Note: Even if it is judged that a frame should not be learned, it should > not be dropped because it is destined for not forwarding layer but higher > layer. See IEEE 802.1Q-2011 8.13.10. > > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> In reference to Vlad's suggestion to try to reuse the logic of the existing br_allowed_ingress() function, I don't think that's so easy. As stated already, it drops packets whilst we don't want that here. Another difference is that it does vlan_untag(), which we also do not want here. Let's just stay with this version of the fix, Vlad if you're OK with that can you please give your ACK? Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/30/2014 06:48 PM, David Miller wrote: > From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > Date: Mon, 26 May 2014 15:15:53 +0900 > >> br_handle_local_finish() is allowing us to insert an FDB entry with >> disallowed vlan. For example, when port 1 and 2 are communicating in >> vlan 10, and even if vlan 10 is disallowed on port 3, port 3 can >> interfere with their communication by spoofed src mac address with >> vlan id 10. >> >> Note: Even if it is judged that a frame should not be learned, it should >> not be dropped because it is destined for not forwarding layer but higher >> layer. See IEEE 802.1Q-2011 8.13.10. >> >> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > > In reference to Vlad's suggestion to try to reuse the logic of the > existing br_allowed_ingress() function, I don't think that's so > easy. > > As stated already, it drops packets whilst we don't want that here. > > Another difference is that it does vlan_untag(), which we also do > not want here. > > Let's just stay with this version of the fix, Vlad if you're OK with > that can you please give your ACK? Thanks. > Acked-by: Vlad Yasevich <vyasevic@redhat.com> I need to spend a little time and figure out how to make it more re-usable. -vlad -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Vlad Yasevich <vyasevic@redhat.com> Date: Mon, 02 Jun 2014 10:22:10 -0400 > On 05/30/2014 06:48 PM, David Miller wrote: >> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >> Date: Mon, 26 May 2014 15:15:53 +0900 >> >>> br_handle_local_finish() is allowing us to insert an FDB entry with >>> disallowed vlan. For example, when port 1 and 2 are communicating in >>> vlan 10, and even if vlan 10 is disallowed on port 3, port 3 can >>> interfere with their communication by spoofed src mac address with >>> vlan id 10. >>> >>> Note: Even if it is judged that a frame should not be learned, it should >>> not be dropped because it is destined for not forwarding layer but higher >>> layer. See IEEE 802.1Q-2011 8.13.10. >>> >>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >> >> In reference to Vlad's suggestion to try to reuse the logic of the >> existing br_allowed_ingress() function, I don't think that's so >> easy. >> >> As stated already, it drops packets whilst we don't want that here. >> >> Another difference is that it does vlan_untag(), which we also do >> not want here. >> >> Let's just stay with this version of the fix, Vlad if you're OK with >> that can you please give your ACK? Thanks. >> > > > Acked-by: Vlad Yasevich <vyasevic@redhat.com> Applied, thanks everyone. > I need to spend a little time and figure out how to make it more re-usable. Ok. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 7985dea..04d6348 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -147,8 +147,8 @@ static int br_handle_local_finish(struct sk_buff *skb) struct net_bridge_port *p = br_port_get_rcu(skb->dev); u16 vid = 0; - br_vlan_get_tag(skb, &vid); - if (p->flags & BR_LEARNING) + /* check if vlan is allowed, to avoid spoofing */ + if (p->flags & BR_LEARNING && br_should_learn(p, skb, &vid)) br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, false); return 0; /* process further */ } diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 06811d7..59d3a85 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -581,6 +581,7 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v, struct sk_buff *skb, u16 *vid); bool br_allowed_egress(struct net_bridge *br, const struct net_port_vlans *v, const struct sk_buff *skb); +bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid); struct sk_buff *br_handle_vlan(struct net_bridge *br, const struct net_port_vlans *v, struct sk_buff *skb); @@ -648,6 +649,12 @@ static inline bool br_allowed_egress(struct net_bridge *br, return true; } +static inline bool br_should_learn(struct net_bridge_port *p, + struct sk_buff *skb, u16 *vid) +{ + return true; +} + static inline struct sk_buff *br_handle_vlan(struct net_bridge *br, const struct net_port_vlans *v, struct sk_buff *skb) diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 4a37161..5fee2fe 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -241,6 +241,34 @@ bool br_allowed_egress(struct net_bridge *br, return false; } +/* Called under RCU */ +bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid) +{ + struct net_bridge *br = p->br; + struct net_port_vlans *v; + + if (!br->vlan_enabled) + return true; + + v = rcu_dereference(p->vlan_info); + if (!v) + return false; + + br_vlan_get_tag(skb, vid); + if (!*vid) { + *vid = br_get_pvid(v); + if (*vid == VLAN_N_VID) + return false; + + return true; + } + + if (test_bit(*vid, v->vlan_bitmap)) + return true; + + return false; +} + /* Must be protected by RTNL. * Must be called with vid in range from 1 to 4094 inclusive. */
br_handle_local_finish() is allowing us to insert an FDB entry with disallowed vlan. For example, when port 1 and 2 are communicating in vlan 10, and even if vlan 10 is disallowed on port 3, port 3 can interfere with their communication by spoofed src mac address with vlan id 10. Note: Even if it is judged that a frame should not be learned, it should not be dropped because it is destined for not forwarding layer but higher layer. See IEEE 802.1Q-2011 8.13.10. Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> --- net/bridge/br_input.c | 4 ++-- net/bridge/br_private.h | 7 +++++++ net/bridge/br_vlan.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-)