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

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZCWT4tK6aNBshFcl@corigine.com>
Date:   Thu, 30 Mar 2023 15:51:30 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, "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: Re: [PATCH net-next] net: dsa: fix db type confusion in host fdb/mdb
 add/del

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@....com>

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