netdev - [PATCH net] net: dsa: fix panic when port leaves a bridge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 14 Mar 2022 16:34:10 +0100
From:   Marek Behún <kabel@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>,
        Tobias Waldekranz <tobias@...dekranz.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Marek Behún <kabel@...nel.org>,
        Jan Bětík <hagrid@...ne.us>
Subject: [PATCH net] net: dsa: fix panic when port leaves a bridge

Fix a data structure breaking / NULL-pointer dereference in
dsa_switch_bridge_leave().

When a DSA port leaves a bridge, dsa_switch_bridge_leave() is called by
notifier for every DSA switch that contains ports that are in the
bridge.

But the part of the code that unsets vlan_filtering expects that the ds
argument refers to the same switch that contains the leaving port.

This leads to various problems, including a NULL pointer dereference,
which was observed on Turris MOX with 2 switches (one with 8 user ports
and another with 4 user ports).

Thus we need to move the vlan_filtering change code to the non-crosschip
branch.

Fixes: d371b7c92d190 ("net: dsa: Unset vlan_filtering when ports leave the bridge")
Reported-by: Jan Bětík <hagrid@...ne.us>
Signed-off-by: Marek Behún <kabel@...nel.org>
---
 net/dsa/switch.c | 97 +++++++++++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 42 deletions(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index e3c7d2627a61..38afb1e8fcb7 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -123,9 +123,61 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 	struct dsa_port *dp;
 	int err;
 
-	if (dst->index == info->tree_index && ds->index == info->sw_index &&
-	    ds->ops->port_bridge_leave)
-		ds->ops->port_bridge_leave(ds, info->port, info->bridge);
+	if (dst->index == info->tree_index && ds->index == info->sw_index) {
+		if (ds->ops->port_bridge_leave)
+			ds->ops->port_bridge_leave(ds, info->port,
+						   info->bridge);
+
+		/* This function is called by the notifier for every DSA switch
+		 * that has ports in the bridge we are leaving, but vlan
+		 * filtering on the port should be changed only once. Since the
+		 * code expects that ds is the switch containing the leaving
+		 * port, the following code must not be called in the crosschip
+		 * branch, only here.
+		 */
+
+		if (ds->needs_standalone_vlan_filtering &&
+		    !br_vlan_enabled(info->bridge.dev)) {
+			change_vlan_filtering = true;
+			vlan_filtering = true;
+		} else if (!ds->needs_standalone_vlan_filtering &&
+			   br_vlan_enabled(info->bridge.dev)) {
+			change_vlan_filtering = true;
+			vlan_filtering = false;
+		}
+
+		/* If the bridge was vlan_filtering, the bridge core doesn't
+		 * trigger an event for changing vlan_filtering setting upon
+		 * slave ports leaving it. That is a good thing, because that
+		 * lets us handle it and also handle the case where the switch's
+		 * vlan_filtering setting is global (not per port). When that
+		 * happens, the correct moment to trigger the vlan_filtering
+		 * callback is only when the last port leaves the last
+		 * VLAN-aware bridge.
+		 */
+		if (change_vlan_filtering && ds->vlan_filtering_is_global) {
+			dsa_switch_for_each_port(dp, ds) {
+				struct net_device *br =
+					dsa_port_bridge_dev_get(dp);
+
+				if (br && br_vlan_enabled(br)) {
+					change_vlan_filtering = false;
+					break;
+				}
+			}
+		}
+
+		if (change_vlan_filtering) {
+			err = dsa_port_vlan_filtering(dsa_to_port(ds,
+								  info->port),
+						      vlan_filtering, &extack);
+			if (extack._msg)
+				dev_err(ds->dev, "port %d: %s\n", info->port,
+					extack._msg);
+			if (err && err != -EOPNOTSUPP)
+				return err;
+		}
+	}
 
 	if ((dst->index != info->tree_index || ds->index != info->sw_index) &&
 	    ds->ops->crosschip_bridge_leave)
@@ -133,45 +185,6 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 						info->sw_index, info->port,
 						info->bridge);
 
-	if (ds->needs_standalone_vlan_filtering &&
-	    !br_vlan_enabled(info->bridge.dev)) {
-		change_vlan_filtering = true;
-		vlan_filtering = true;
-	} else if (!ds->needs_standalone_vlan_filtering &&
-		   br_vlan_enabled(info->bridge.dev)) {
-		change_vlan_filtering = true;
-		vlan_filtering = false;
-	}
-
-	/* If the bridge was vlan_filtering, the bridge core doesn't trigger an
-	 * event for changing vlan_filtering setting upon slave ports leaving
-	 * it. That is a good thing, because that lets us handle it and also
-	 * handle the case where the switch's vlan_filtering setting is global
-	 * (not per port). When that happens, the correct moment to trigger the
-	 * vlan_filtering callback is only when the last port leaves the last
-	 * VLAN-aware bridge.
-	 */
-	if (change_vlan_filtering && ds->vlan_filtering_is_global) {
-		dsa_switch_for_each_port(dp, ds) {
-			struct net_device *br = dsa_port_bridge_dev_get(dp);
-
-			if (br && br_vlan_enabled(br)) {
-				change_vlan_filtering = false;
-				break;
-			}
-		}
-	}
-
-	if (change_vlan_filtering) {
-		err = dsa_port_vlan_filtering(dsa_to_port(ds, info->port),
-					      vlan_filtering, &extack);
-		if (extack._msg)
-			dev_err(ds->dev, "port %d: %s\n", info->port,
-				extack._msg);
-		if (err && err != -EOPNOTSUPP)
-			return err;
-	}
-
 	return dsa_tag_8021q_bridge_leave(ds, info);
 }
 
-- 
2.34.1