[PATCH net 0/2] net: bridge: vlan: fix memory leak & null pointer deref

[PATCH net 0/2] net: bridge: vlan: fix memory leak & null pointer deref

[Nikolay Aleksandrov]

Hi,
These are two issues I found while reworking the bridge vlan fast-path.
I need to fix them before I can post my larger patch-sets because they
will make these bugs more visible and easy to trigger.

Patch 01 fixes memory leaks on the vlan flush path (teardown). These have
existed since I converted the VLAN code to use rhashtables, but are hard
to hit because they require __vlan_vid_del or switch dev error to hit.
It is ok to abort vlan deletion when removing a single vlan, but we must
free their resources on teardown otherwise they're left dangling.

Patch 02 fixes a NULL pointer deref that is a consequence of patch 01's
bug. If the vlan delete fails when removing a bridge device vlan, the
error printing will try to dereference the port which is NULL.

Thanks,
 Nik

Nikolay Aleksandrov (2):
  net: bridge: vlan: fix leaks on switchdev deletion errors
  net: bridge: vlan: avoid NULL dereference on flush errors

 net/bridge/br_vlan.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

-- 
2.47.3

[PATCH net 1/2] net: bridge: vlan: fix leaks on switchdev deletion errors

[Nikolay Aleksandrov]

__vlan_del() stops the software deletion when the switchdev operation
fails which is ok for an explicit VLAN deletion because the VLAN remains
configured but not ok when its VLAN group is being destroyed and everything
is being freed. __vlan_flush() always destroys the VLAN group after walking
it regardless of individual deletion errors, so aborting the software vlan
delete leaks the VLAN object's memory (and potentially its master VLAN, due
to references).

Allow teardown callers to finish the software deletion while preserving
error for reporting. Save the VLAN id before deleting because the VLAN
object can already be freed (queued for freeing by call_rcu).

Fixes: 2594e9064a57 ("bridge: vlan: add per-vlan struct and move to rhashtables")
Fixes: 9c86ce2c1ae3 ("net: bridge: Notify about bridge VLANs")
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_vlan.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1e0e436629ec..1748ea1fc202 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -387,7 +387,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
 	goto out;
 }
 
-static int __vlan_del(struct net_bridge_vlan *v)
+static int __vlan_del(struct net_bridge_vlan *v, bool teardown)
 {
 	struct net_bridge_vlan *masterv = v;
 	struct net_bridge_vlan_group *vg;
@@ -405,13 +405,14 @@ static int __vlan_del(struct net_bridge_vlan *v)
 	__vlan_delete_pvid(vg, v->vid);
 	if (p) {
 		err = __vlan_vid_del(p->dev, p->br, v);
-		if (err)
+		if (err && !teardown)
 			goto out;
 	} else {
 		err = br_switchdev_port_vlan_del(v->br->dev, v->vid);
-		if (err && err != -EOPNOTSUPP)
+		if (err == -EOPNOTSUPP)
+			err = 0;
+		else if (err && !teardown)
 			goto out;
-		err = 0;
 	}
 
 	if (br_vlan_should_use(v)) {
@@ -448,7 +449,7 @@ static void __vlan_flush(const struct net_bridge *br,
 			 struct net_bridge_vlan_group *vg)
 {
 	struct net_bridge_vlan *vlan, *tmp;
-	u16 v_start = 0, v_end = 0;
+	u16 v_start = 0, v_end = 0, vid;
 	int err;
 
 	__vlan_delete_pvid(vg, vg->pvid);
@@ -463,12 +464,13 @@ static void __vlan_flush(const struct net_bridge *br,
 		}
 		v_end = vlan->vid;
 
-		err = __vlan_del(vlan);
+		vid = vlan->vid;
+		err = __vlan_del(vlan, true);
 		if (err) {
 			br_err(br,
 			       "port %u(%s) failed to delete vlan %d: %pe\n",
 			       (unsigned int) p->port_no, p->dev->name,
-			       vlan->vid, ERR_PTR(err));
+			       vid, ERR_PTR(err));
 		}
 	}
 
@@ -838,7 +840,7 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
 
 	vlan_tunnel_info_del(vg, v);
 
-	return __vlan_del(v);
+	return __vlan_del(v, false);
 }
 
 void br_vlan_flush(struct net_bridge *br)
@@ -1369,7 +1371,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	br_fdb_find_delete_local(port->br, port, port->dev->dev_addr, vid);
 	br_fdb_delete_by_port(port->br, port, vid, 0);
 
-	return __vlan_del(v);
+	return __vlan_del(v, false);
 }
 
 void nbp_vlan_flush(struct net_bridge_port *port)
-- 
2.47.3

[PATCH net 2/2] net: bridge: vlan: avoid NULL dereference on flush errors

[Nikolay Aleksandrov]

__vlan_flush() is used for both port and bridge VLAN groups. The error
path unconditionally dereferences the port argument even though
br_vlan_flush() calls it with a NULL port. Any error (e.g. switchdev) while
deleting a bridge VLAN can result in a NULL pointer dereference.
Use a bridge-specific error message when called for the bridge device.

Fixes: 5454f5c28eca ("net: bridge: vlan: check for errors from __vlan_del in __vlan_flush")
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_vlan.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1748ea1fc202..3aa0e1fedeb2 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -467,10 +467,15 @@ static void __vlan_flush(const struct net_bridge *br,
 		vid = vlan->vid;
 		err = __vlan_del(vlan, true);
 		if (err) {
-			br_err(br,
-			       "port %u(%s) failed to delete vlan %d: %pe\n",
-			       (unsigned int) p->port_no, p->dev->name,
-			       vid, ERR_PTR(err));
+			if (p)
+				br_err(br,
+				       "port %u(%s) failed to delete vlan %d: %pe\n",
+				       (unsigned int)p->port_no, p->dev->name,
+				       vid, ERR_PTR(err));
+			else
+				br_err(br,
+				       "failed to delete bridge vlan %d: %pe\n",
+				       vid, ERR_PTR(err));
 		}
 	}
 
-- 
2.47.3