| 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
| ||
|
Message-Id: <20230329133819.697642-1-vladimir.oltean@nxp.com>
Date: Wed, 29 Mar 2023 16:38:19 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>
Subject: [PATCH net-next] net: dsa: fix db type confusion in host fdb/mdb add/del
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@....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
Powered by blists - more mailing lists