netdev - Re: [PATCH net-next v4 3/6] net: dsa: Introduce dsa tagger data operation.

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: <20220906135153.kkvw4oxe34or5dai@skbuf>
Date:   Tue, 6 Sep 2022 16:51:53 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Mattias Forsblad <mattias.forsblad@...il.com>,
        netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v4 3/6] net: dsa: Introduce dsa tagger data
 operation.

On Tue, Sep 06, 2022 at 03:08:23PM +0200, Andrew Lunn wrote:
> >  		switch (code) {
> >  		case DSA_CODE_FRAME2REG:
> > -			/* Remote management is not implemented yet,
> > -			 * drop.
> > -			 */
> > +			tagger_data = ds->tagger_data;
> > +			if (likely(tagger_data->decode_frame2reg))
> > +				tagger_data->decode_frame2reg(dev, skb);
> >  			return NULL;
> >  		case DSA_CODE_ARP_MIRROR:
> >  		case DSA_CODE_POLICY_MIRROR:
> > @@ -323,6 +326,25 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
> >  	return skb;
> >  }
>   
> > +static void dsa_tag_disconnect(struct dsa_switch *ds)
> > +{
> > +	kfree(ds->tagger_data);
> > +	ds->tagger_data = NULL;
> > +}
> 
> 
> Probably a question for Vladimir.
> 
> Is there a potential use after free here? Is it guaranteed that the
> masters dev->dsa_ptr will be cleared before the disconnect function is
> called?

There is no use after free because there is no free...
There are 3 cases of tag protocol disconnect, one is on error path of
bidirectional connection (handled properly), another is on tag protocol
change (handled properly; we only allow it with the master down), and
another is on switch driver removal (handled incorrectly, we don't do
anything here).

The following patch is compile tested only. However it should answer
your question of order of operations too.

>From d93b2ddf0e0f4e82d6b0bac4591b346e8cd0fdb9 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@....com>
Date: Tue, 6 Sep 2022 16:24:41 +0300
Subject: [PATCH] net: dsa: don't leak tagger-owned storage on switch driver
 unbind

In the initial commit dc452a471dba ("net: dsa: introduce tagger-owned
storage for private and shared data"), we had a call to
tag_ops->disconnect(dst) issued from dsa_tree_free(), which is called at
tree teardown time.

There were problems with connecting to a switch tree as a whole, so this
got reworked to connecting to individual switches within the tree. In
this process, tag_ops->disconnect(ds) was made to be called only from
switch.c (cross-chip notifiers emitted as a result of dynamic tag proto
changes), but the normal driver teardown code path wasn't replaced with
anything.

Solve this problem by adding a function that does the opposite of
dsa_switch_setup_tag_protocol(), which is called from the equivalent
spot in dsa_switch_teardown(). The positioning here also ensures that we
won't have any use-after-free in tagging protocol (*rcv) ops, since the
teardown sequence is as follows:

dsa_tree_teardown
-> dsa_tree_teardown_master
   -> dsa_master_teardown
      -> unsets master->dsa_ptr, making no further packets match the
         ETH_P_XDSA packet type handler
-> dsa_tree_teardown_ports
   -> dsa_port_teardown
      -> dsa_slave_destroy
         -> unregisters DSA net devices, there is even a synchronize_net()
            in unregister_netdevice_many()
-> dsa_tree_teardown_switches
   -> dsa_switch_teardown
      -> dsa_switch_teardown_tag_protocol
         -> finally frees the tagger-owned storage

Fixes: 7f2973149c22 ("net: dsa: make tagging protocols connect to individual switches from a tree")
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 net/dsa/dsa2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index ed56c7a554b8..4bb0a203b85c 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -864,6 +864,14 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
 	return err;
 }
 
+static void dsa_switch_teardown_tag_protocol(struct dsa_switch *ds)
+{
+	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
+
+	if (tag_ops->disconnect)
+		tag_ops->disconnect(ds);
+}
+
 static int dsa_switch_setup(struct dsa_switch *ds)
 {
 	struct dsa_devlink_priv *dl_priv;
@@ -967,6 +975,8 @@ static void dsa_switch_teardown(struct dsa_switch *ds)
 		ds->slave_mii_bus = NULL;
 	}
 
+	dsa_switch_teardown_tag_protocol(ds);
+
 	if (ds->ops->teardown)
 		ds->ops->teardown(ds);
 
-- 
2.34.1

> 
> Also, the receive function checks for tagger_data->decode_frame2reg,
> but does not check for tagger_data != NULL.
> 
> This is just a straight copy of tag_qca, so if there are issues here,
> they are probably not new issues.

It checks for tagger_data->decode_frame2reg because the "dsa" or "edsa"
tagging protocol drivers could also be connected to the dsa_loop driver,
for testing. That driver won't populate tagger_data->decode_frame2reg().
What is supposed to happen is that if the tagging protocol driver has no
subscriber for management Ethernet frames, nothing happens with them,
they are just freed right away. This is the model of separate switch
driver and tag protocol driver.