diff mbox series

[RFC,net,2/4] net: dsa: prevent hardware forwarding between unbridged 8021q uppers

Message ID 20210309021657.3639745-3-olteanv@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Clear rx-vlan-filter feature in DSA when necessary | expand

Checks

Context Check Description
netdev/apply fail Patch does not apply to net
netdev/tree_selection success Clearly marked for net

Commit Message

Vladimir Oltean March 9, 2021, 2:16 a.m. UTC 
From: Vladimir Oltean <vladimir.oltean@nxp.com>

Tobias reports that the following set of commands, which bridge two
ports that have 8021q uppers with the same VID, is incorrectly accepted
by DSA as valid:

.100  br0  .100
   \  / \  /
   lan0 lan1

ip link add dev br0 type bridge vlan_filtering 1
ip link add dev lan0.100 link lan0 type vlan id 100
ip link add dev lan1.100 link lan1 type vlan id 100
ip link set dev lan0 master br0
ip link set dev lan1 master br0 # This should fail but doesn't

Again, this is a variation of the same theme of 'all VLANs kinda smell
the same in hardware, you can't tell if they came from 8021q or from the
bridge'. When the base interfaces are bridged, the expectation of the
Linux network stack is that traffic received by other upper interfaces
except the bridge is not captured by the bridge rx_handler, therefore
not subject to forwarding. So the above setup should not do forwarding
for VLAN ID 100, but it does it nonetheless. So it should be denied.

Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
Fixes: 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h |  6 +++++-
 net/dsa/port.c     | 23 ++++++++++++++++++++++-
 net/dsa/slave.c    | 13 +++++++++----
 3 files changed, 36 insertions(+), 6 deletions(-)

Comments

Tobias Waldekranz March 9, 2021, 6:56 p.m. UTC | #1 
On Tue, Mar 09, 2021 at 04:16, Vladimir Oltean <olteanv@gmail.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> Tobias reports that the following set of commands, which bridge two
> ports that have 8021q uppers with the same VID, is incorrectly accepted
> by DSA as valid:
>
> .100  br0  .100
>    \  / \  /
>    lan0 lan1
>
> ip link add dev br0 type bridge vlan_filtering 1
> ip link add dev lan0.100 link lan0 type vlan id 100
> ip link add dev lan1.100 link lan1 type vlan id 100

If I move this line...

> ip link set dev lan0 master br0
> ip link set dev lan1 master br0 # This should fail but doesn't

...down here, the config is (erroneously) accepted.

> Again, this is a variation of the same theme of 'all VLANs kinda smell
> the same in hardware, you can't tell if they came from 8021q or from the
> bridge'. When the base interfaces are bridged, the expectation of the
> Linux network stack is that traffic received by other upper interfaces
> except the bridge is not captured by the bridge rx_handler, therefore
> not subject to forwarding. So the above setup should not do forwarding
> for VLAN ID 100, but it does it nonetheless. So it should be denied.
>
> Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
> Fixes: 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

This is what I meant by having bits and pieces of this validation
scattered in multiple places, some things being checked for certain
events but not for others, etc.

I took an initial stab at this to show what I mean:

https://lore.kernel.org/netdev/20210309184244.1970173-1-tobias@waldekranz.com

I am sure there are holes in this as well, hence RFC, but I think it
will be much easier to make sure that we avoid ordering issues using a
structure like this.

What do you think?
diff mbox series

Patch

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index d40dfede494c..d6f9b73241b4 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -181,7 +181,8 @@  int dsa_port_enable_rt(struct dsa_port *dp, struct phy_device *phy);
 int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy);
 void dsa_port_disable_rt(struct dsa_port *dp);
 void dsa_port_disable(struct dsa_port *dp);
-int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br);
+int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
+			 struct netlink_ext_ack *extack);
 void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br);
 int dsa_port_lag_change(struct dsa_port *dp,
 			struct netdev_lag_lower_state_info *linfo);
@@ -261,6 +262,9 @@  static inline bool dsa_tree_offloads_netdev(struct dsa_switch_tree *dst,
 
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
+int dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
+						  struct net_device *skip,
+						  u16 vid);
 void dsa_slave_mii_bus_init(struct dsa_switch *ds);
 int dsa_slave_create(struct dsa_port *dp);
 void dsa_slave_destroy(struct net_device *slave_dev);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index c9c6d7ab3f47..0aad5a84361c 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -144,7 +144,8 @@  static void dsa_port_change_brport_flags(struct dsa_port *dp,
 	}
 }
 
-int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
+int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
+			 struct netlink_ext_ack *extack)
 {
 	struct dsa_notifier_bridge_info info = {
 		.tree_index = dp->ds->dst->index,
@@ -152,8 +153,28 @@  int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
 		.port = dp->index,
 		.br = br,
 	};
+	struct net_device *slave = dp->slave;
+	struct net_device *upper_dev;
+	struct list_head *iter;
 	int err;
 
+	netdev_for_each_upper_dev_rcu(slave, upper_dev, iter) {
+		u16 vid;
+
+		if (!is_vlan_dev(upper_dev))
+			continue;
+
+		vid = vlan_dev_vlan_id(upper_dev);
+
+		err = dsa_check_bridge_for_overlapping_8021q_uppers(br, slave,
+								    vid);
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Configuration would leak VLAN-tagged packets between bridge ports");
+			return err;
+		}
+	}
+
 	/* Notify the port driver to set its configurable flags in a way that
 	 * matches the initial settings of a bridge port.
 	 */
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d36e11399626..0e884fd439f8 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -323,9 +323,9 @@  static int dsa_slave_port_attr_set(struct net_device *dev,
 	return ret;
 }
 
-static int
-dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
-					      u16 vid)
+int dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
+						  struct net_device *skip,
+						  u16 vid)
 {
 	struct list_head *iter_upper, *iter_lower;
 	struct net_device *upper, *lower;
@@ -334,6 +334,9 @@  dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
 		if (!dsa_slave_dev_check(lower))
 			continue;
 
+		if (lower == skip)
+			continue;
+
 		netdev_for_each_upper_dev_rcu(lower, upper, iter_upper) {
 			u16 upper_vid;
 
@@ -373,6 +376,7 @@  static int dsa_slave_vlan_add(struct net_device *dev,
 	 */
 	if (br_vlan_enabled(dp->bridge_dev)) {
 		err = dsa_check_bridge_for_overlapping_8021q_uppers(dp->bridge_dev,
+								    NULL,
 								    vlan.vid);
 		if (err) {
 			NL_SET_ERR_MSG_MOD(extack,
@@ -1969,7 +1973,8 @@  static int dsa_slave_changeupper(struct net_device *dev,
 
 	if (netif_is_bridge_master(info->upper_dev)) {
 		if (info->linking) {
-			err = dsa_port_bridge_join(dp, info->upper_dev);
+			err = dsa_port_bridge_join(dp, info->upper_dev,
+						   info->info.extack);
 			if (!err)
 				dsa_bridge_mtu_normalization(dp);
 			err = notifier_from_errno(err);