[PATCH net-next] net: dsa: fix db type confusion in host fdb/mdb add/del

[PATCH net-next] net: dsa: fix db type confusion in host fdb/mdb add/del

[Vladimir Oltean]

We have the following code paths:

Host FDB (unicast RX filtering):

dsa_port_standalone_host_fdb_add()   dsa_port_bridge_host_fdb_add()
               |                                     |
               +--------------+         +------------+
                              |         |
                              v         v
                         dsa_port_host_fdb_add()

dsa_port_standalone_host_fdb_del()   dsa_port_bridge_host_fdb_del()
               |                                     |
               +--------------+         +------------+
                              |         |
                              v         v
                         dsa_port_host_fdb_del()

Host MDB (multicast RX filtering):

dsa_port_standalone_host_mdb_add()   dsa_port_bridge_host_mdb_add()
               |                                     |
               +--------------+         +------------+
                              |         |
                              v         v
                         dsa_port_host_mdb_add()

dsa_port_standalone_host_mdb_del()   dsa_port_bridge_host_mdb_del()
               |                                     |
               +--------------+         +------------+
                              |         |
                              v         v
                         dsa_port_host_mdb_del()

The logic added by commit 5e8a1e03aa4d ("net: dsa: install secondary
unicast and multicast addresses as host FDB/MDB") zeroes out
db.bridge.num if the switch doesn't support ds->fdb_isolation
(the majority doesn't). This is done for a reason explained in commit
c26933639b54 ("net: dsa: request drivers to perform FDB isolation").

Taking a single code path as example - dsa_port_host_fdb_add() - the
others are similar - the problem is that this function handles:
- DSA_DB_PORT databases, when called from
  dsa_port_standalone_host_fdb_add()
- DSA_DB_BRIDGE databases, when called from
  dsa_port_bridge_host_fdb_add()

So, if dsa_port_host_fdb_add() were to make any change on the
"bridge.num" attribute of the database, this would only be correct for a
DSA_DB_BRIDGE, and a type confusion for a DSA_DB_PORT bridge.

However, this bug is without consequences, for 2 reasons:

- dsa_port_standalone_host_fdb_add() is only called from code which is
  (in)directly guarded by dsa_switch_supports_uc_filtering(ds), and that
  function only returns true if ds->fdb_isolation is set. So, the code
  only executed for DSA_DB_BRIDGE databases.

- Even if the code was not dead for DSA_DB_PORT, we have the following
  memory layout:

struct dsa_bridge {
	struct net_device *dev;
	unsigned int num;
	bool tx_fwd_offload;
	refcount_t refcount;
};

struct dsa_db {
	enum dsa_db_type type;

	union {
		const struct dsa_port *dp; // DSA_DB_PORT
		struct dsa_lag lag;
		struct dsa_bridge bridge; // DSA_DB_BRIDGE
	};
};

So, the zeroization of dsa_db :: bridge :: num on a dsa_db structure of
type DSA_DB_PORT would access memory which is unused, because we only
use dsa_db :: dp for DSA_DB_PORT, and this is mapped at the same address
with dsa_db :: dev for DSA_DB_BRIDGE, thanks to the union definition.

It is correct to fix up dsa_db :: bridge :: num only from code paths
that come from the bridge / switchdev, so move these there.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/port.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 67ad1adec2a2..15cee17769e9 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1028,9 +1028,6 @@ static int dsa_port_host_fdb_add(struct dsa_port *dp,
 		.db = db,
 	};
 
-	if (!dp->ds->fdb_isolation)
-		info.db.bridge.num = 0;
-
 	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_FDB_ADD, &info);
 }
 
@@ -1055,6 +1052,9 @@ int dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
 	};
 	int err;
 
+	if (!dp->ds->fdb_isolation)
+		db.bridge.num = 0;
+
 	/* Avoid a call to __dev_set_promiscuity() on the master, which
 	 * requires rtnl_lock(), since we can't guarantee that is held here,
 	 * and we can't take it either.
@@ -1079,9 +1079,6 @@ static int dsa_port_host_fdb_del(struct dsa_port *dp,
 		.db = db,
 	};
 
-	if (!dp->ds->fdb_isolation)
-		info.db.bridge.num = 0;
-
 	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_FDB_DEL, &info);
 }
 
@@ -1106,6 +1103,9 @@ int dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
 	};
 	int err;
 
+	if (!dp->ds->fdb_isolation)
+		db.bridge.num = 0;
+
 	if (master->priv_flags & IFF_UNICAST_FLT) {
 		err = dev_uc_del(master, addr);
 		if (err)
@@ -1210,9 +1210,6 @@ static int dsa_port_host_mdb_add(const struct dsa_port *dp,
 		.db = db,
 	};
 
-	if (!dp->ds->fdb_isolation)
-		info.db.bridge.num = 0;
-
 	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_ADD, &info);
 }
 
@@ -1237,6 +1234,9 @@ int dsa_port_bridge_host_mdb_add(const struct dsa_port *dp,
 	};
 	int err;
 
+	if (!dp->ds->fdb_isolation)
+		db.bridge.num = 0;
+
 	err = dev_mc_add(master, mdb->addr);
 	if (err)
 		return err;
@@ -1254,9 +1254,6 @@ static int dsa_port_host_mdb_del(const struct dsa_port *dp,
 		.db = db,
 	};
 
-	if (!dp->ds->fdb_isolation)
-		info.db.bridge.num = 0;
-
 	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_DEL, &info);
 }
 
@@ -1281,6 +1278,9 @@ int dsa_port_bridge_host_mdb_del(const struct dsa_port *dp,
 	};
 	int err;
 
+	if (!dp->ds->fdb_isolation)
+		db.bridge.num = 0;
+
 	err = dev_mc_del(master, mdb->addr);
 	if (err)
 		return err;
-- 
2.34.1

Re: [PATCH net-next] net: dsa: fix db type confusion in host fdb/mdb add/del

[Simon Horman]

On Wed, Mar 29, 2023 at 04:38:19PM +0300, Vladimir Oltean wrote:
> We have the following code paths:
> 
> Host FDB (unicast RX filtering):
> 
> dsa_port_standalone_host_fdb_add()   dsa_port_bridge_host_fdb_add()
>                |                                     |
>                +--------------+         +------------+
>                               |         |
>                               v         v
>                          dsa_port_host_fdb_add()
> 
> dsa_port_standalone_host_fdb_del()   dsa_port_bridge_host_fdb_del()
>                |                                     |
>                +--------------+         +------------+
>                               |         |
>                               v         v
>                          dsa_port_host_fdb_del()
> 
> Host MDB (multicast RX filtering):
> 
> dsa_port_standalone_host_mdb_add()   dsa_port_bridge_host_mdb_add()
>                |                                     |
>                +--------------+         +------------+
>                               |         |
>                               v         v
>                          dsa_port_host_mdb_add()
> 
> dsa_port_standalone_host_mdb_del()   dsa_port_bridge_host_mdb_del()
>                |                                     |
>                +--------------+         +------------+
>                               |         |
>                               v         v
>                          dsa_port_host_mdb_del()
> 
> The logic added by commit 5e8a1e03aa4d ("net: dsa: install secondary
> unicast and multicast addresses as host FDB/MDB") zeroes out
> db.bridge.num if the switch doesn't support ds->fdb_isolation
> (the majority doesn't). This is done for a reason explained in commit
> c26933639b54 ("net: dsa: request drivers to perform FDB isolation").
> 
> Taking a single code path as example - dsa_port_host_fdb_add() - the
> others are similar - the problem is that this function handles:
> - DSA_DB_PORT databases, when called from
>   dsa_port_standalone_host_fdb_add()
> - DSA_DB_BRIDGE databases, when called from
>   dsa_port_bridge_host_fdb_add()
> 
> So, if dsa_port_host_fdb_add() were to make any change on the
> "bridge.num" attribute of the database, this would only be correct for a
> DSA_DB_BRIDGE, and a type confusion for a DSA_DB_PORT bridge.
> 
> However, this bug is without consequences, for 2 reasons:
> 
> - dsa_port_standalone_host_fdb_add() is only called from code which is
>   (in)directly guarded by dsa_switch_supports_uc_filtering(ds), and that
>   function only returns true if ds->fdb_isolation is set. So, the code
>   only executed for DSA_DB_BRIDGE databases.
> 
> - Even if the code was not dead for DSA_DB_PORT, we have the following
>   memory layout:
> 
> struct dsa_bridge {
> 	struct net_device *dev;
> 	unsigned int num;
> 	bool tx_fwd_offload;
> 	refcount_t refcount;
> };
> 
> struct dsa_db {
> 	enum dsa_db_type type;
> 
> 	union {
> 		const struct dsa_port *dp; // DSA_DB_PORT
> 		struct dsa_lag lag;
> 		struct dsa_bridge bridge; // DSA_DB_BRIDGE
> 	};
> };
> 
> So, the zeroization of dsa_db :: bridge :: num on a dsa_db structure of
> type DSA_DB_PORT would access memory which is unused, because we only
> use dsa_db :: dp for DSA_DB_PORT, and this is mapped at the same address
> with dsa_db :: dev for DSA_DB_BRIDGE, thanks to the union definition.
> 
> It is correct to fix up dsa_db :: bridge :: num only from code paths
> that come from the bridge / switchdev, so move these there.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next] net: dsa: fix db type confusion in host fdb/mdb add/del

[Florian Fainelli]

On 3/29/23 06:38, Vladimir Oltean wrote:
> We have the following code paths:
> 
> Host FDB (unicast RX filtering):
> 
> dsa_port_standalone_host_fdb_add()   dsa_port_bridge_host_fdb_add()
>                 |                                     |
>                 +--------------+         +------------+
>                                |         |
>                                v         v
>                           dsa_port_host_fdb_add()
> 
> dsa_port_standalone_host_fdb_del()   dsa_port_bridge_host_fdb_del()
>                 |                                     |
>                 +--------------+         +------------+
>                                |         |
>                                v         v
>                           dsa_port_host_fdb_del()
> 
> Host MDB (multicast RX filtering):
> 
> dsa_port_standalone_host_mdb_add()   dsa_port_bridge_host_mdb_add()
>                 |                                     |
>                 +--------------+         +------------+
>                                |         |
>                                v         v
>                           dsa_port_host_mdb_add()
> 
> dsa_port_standalone_host_mdb_del()   dsa_port_bridge_host_mdb_del()
>                 |                                     |
>                 +--------------+         +------------+
>                                |         |
>                                v         v
>                           dsa_port_host_mdb_del()
> 
> The logic added by commit 5e8a1e03aa4d ("net: dsa: install secondary
> unicast and multicast addresses as host FDB/MDB") zeroes out
> db.bridge.num if the switch doesn't support ds->fdb_isolation
> (the majority doesn't). This is done for a reason explained in commit
> c26933639b54 ("net: dsa: request drivers to perform FDB isolation").
> 
> Taking a single code path as example - dsa_port_host_fdb_add() - the
> others are similar - the problem is that this function handles:
> - DSA_DB_PORT databases, when called from
>    dsa_port_standalone_host_fdb_add()
> - DSA_DB_BRIDGE databases, when called from
>    dsa_port_bridge_host_fdb_add()
> 
> So, if dsa_port_host_fdb_add() were to make any change on the
> "bridge.num" attribute of the database, this would only be correct for a
> DSA_DB_BRIDGE, and a type confusion for a DSA_DB_PORT bridge.
> 
> However, this bug is without consequences, for 2 reasons:
> 
> - dsa_port_standalone_host_fdb_add() is only called from code which is
>    (in)directly guarded by dsa_switch_supports_uc_filtering(ds), and that
>    function only returns true if ds->fdb_isolation is set. So, the code
>    only executed for DSA_DB_BRIDGE databases.
> 
> - Even if the code was not dead for DSA_DB_PORT, we have the following
>    memory layout:
> 
> struct dsa_bridge {
> 	struct net_device *dev;
> 	unsigned int num;
> 	bool tx_fwd_offload;
> 	refcount_t refcount;
> };
> 
> struct dsa_db {
> 	enum dsa_db_type type;
> 
> 	union {
> 		const struct dsa_port *dp; // DSA_DB_PORT
> 		struct dsa_lag lag;
> 		struct dsa_bridge bridge; // DSA_DB_BRIDGE
> 	};
> };
> 
> So, the zeroization of dsa_db :: bridge :: num on a dsa_db structure of
> type DSA_DB_PORT would access memory which is unused, because we only
> use dsa_db :: dp for DSA_DB_PORT, and this is mapped at the same address
> with dsa_db :: dev for DSA_DB_BRIDGE, thanks to the union definition.
> 
> It is correct to fix up dsa_db :: bridge :: num only from code paths
> that come from the bridge / switchdev, so move these there.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next] net: dsa: fix db type confusion in host fdb/mdb add/del

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 29 Mar 2023 16:38:19 +0300 you wrote:
> We have the following code paths:
> 
> Host FDB (unicast RX filtering):
> 
> dsa_port_standalone_host_fdb_add()   dsa_port_bridge_host_fdb_add()
>                |                                     |
>                +--------------+         +------------+
>                               |         |
>                               v         v
>                          dsa_port_host_fdb_add()
> 
> [...]

Here is the summary with links:
  - [net-next] net: dsa: fix db type confusion in host fdb/mdb add/del
    https://git.kernel.org/netdev/net-next/c/eb1ab7650d35

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html