From eb1ab7650d358b553cc946035fd7c7bdda1856e3 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Wed, 29 Mar 2023 16:38:19 +0300 Subject: 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 Reviewed-by: Simon Horman Reviewed-by: Florian Fainelli Link: https://lore.kernel.org/r/20230329133819.697642-1-vladimir.oltean@nxp.com Signed-off-by: Jakub Kicinski --- net/dsa/port.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'net/dsa/port.c') 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; -- cgit v1.2.3 From ff6ac4d013e680a5e7a38ee83ca59ffe1846915d Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Sun, 2 Apr 2023 15:37:54 +0300 Subject: net: dsa: make dsa_port_supports_hwtstamp() construct a fake ifreq dsa_master_ioctl() is in the process of getting converted to a different API, where we won't have access to a struct ifreq * anymore, but rather, to a struct kernel_hwtstamp_config. Since ds->ops->port_hwtstamp_get() still uses struct ifreq *, this creates a difficult situation where we have to make up such a dummy pointer. The conversion is a bit messy, because it forces a "good" implementation of ds->ops->port_hwtstamp_get() to return -EFAULT in copy_to_user() because of the NULL ifr->ifr_data pointer. However, it works, and it is only a transient step until ds->ops->port_hwtstamp_get() gets converted to the new API which passes struct kernel_hwtstamp_config and does not call copy_to_user(). Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: David S. Miller --- net/dsa/master.c | 2 +- net/dsa/port.c | 10 ++++++---- net/dsa/port.h | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) (limited to 'net/dsa/port.c') diff --git a/net/dsa/master.c b/net/dsa/master.c index 22d3f16b0e6d..e397641382ca 100644 --- a/net/dsa/master.c +++ b/net/dsa/master.c @@ -212,7 +212,7 @@ static int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) * switch in the tree that is PTP capable. */ list_for_each_entry(dp, &dst->ports, list) - if (dsa_port_supports_hwtstamp(dp, ifr)) + if (dsa_port_supports_hwtstamp(dp)) return -EBUSY; break; } diff --git a/net/dsa/port.c b/net/dsa/port.c index 15cee17769e9..71ba30538411 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -114,19 +114,21 @@ static bool dsa_port_can_configure_learning(struct dsa_port *dp) return !err; } -bool dsa_port_supports_hwtstamp(struct dsa_port *dp, struct ifreq *ifr) +bool dsa_port_supports_hwtstamp(struct dsa_port *dp) { struct dsa_switch *ds = dp->ds; + struct ifreq ifr = {}; int err; if (!ds->ops->port_hwtstamp_get || !ds->ops->port_hwtstamp_set) return false; /* "See through" shim implementations of the "get" method. - * This will clobber the ifreq structure, but we will either return an - * error, or the master will overwrite it with proper values. + * Since we can't cook up a complete ioctl request structure, this will + * fail in copy_to_user() with -EFAULT, which hopefully is enough to + * detect a valid implementation. */ - err = ds->ops->port_hwtstamp_get(ds, dp->index, ifr); + err = ds->ops->port_hwtstamp_get(ds, dp->index, &ifr); return err != -EOPNOTSUPP; } diff --git a/net/dsa/port.h b/net/dsa/port.h index 9c218660d223..dc812512fd0e 100644 --- a/net/dsa/port.h +++ b/net/dsa/port.h @@ -15,7 +15,7 @@ struct switchdev_obj_port_mdb; struct switchdev_vlan_msti; struct phy_device; -bool dsa_port_supports_hwtstamp(struct dsa_port *dp, struct ifreq *ifr); +bool dsa_port_supports_hwtstamp(struct dsa_port *dp); void dsa_port_set_tag_protocol(struct dsa_port *cpu_dp, const struct dsa_device_ops *tag_ops); int dsa_port_set_state(struct dsa_port *dp, u8 state, bool do_fast_age); -- cgit v1.2.3