netdev - [PATCH v4 net-next 02/11] net: dsa: mv88e6xxx: deny vid 0 on the CPU port and DSA links too

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: <20210109000156.1246735-3-olteanv@gmail.com>
Date:   Sat,  9 Jan 2021 02:01:47 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Vadym Kochan <vkochan@...vell.com>,
        Taras Chornyi <tchornyi@...vell.com>,
        Jiri Pirko <jiri@...dia.com>, Ido Schimmel <idosch@...dia.com>,
        Grygorii Strashko <grygorii.strashko@...com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Ivan Vecera <ivecera@...hat.com>,
        Petr Machata <petrm@...dia.com>
Subject: [PATCH v4 net-next 02/11] net: dsa: mv88e6xxx: deny vid 0 on the CPU port and DSA links too

From: Vladimir Oltean <vladimir.oltean@....com>

mv88e6xxx apparently has a problem offloading VID 0, which the 8021q
module tries to install as part of commit ad1afb003939 ("vlan_dev: VLAN
0 should be treated as "no vlan tag" (802.1p packet)"). That mv88e6xxx
restriction seems to have been introduced by the "VTU GetNext VID-1
trick to retrieve a single entry" - see commit 2fb5ef09de7c ("net: dsa:
mv88e6xxx: extract single VLAN retrieval").

There is one more problem. The mv88e6xxx CPU port and DSA links do not
report properly in the prepare phase what are the VLANs that they can
offload. They'll say they can offload everything:

mv88e6xxx_port_vlan_prepare
-> mv88e6xxx_port_check_hw_vlan:

	/* DSA and CPU ports have to be members of multiple vlans */
	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
		return 0;

Except that if you actually try to commit to it, they'll error out and
print this message:

[   32.802438] mv88e6085 d0032004.mdio-mii:12: p9: failed to add VLAN 0t

which comes from:

mv88e6xxx_port_vlan_add
-> mv88e6xxx_port_vlan_join:

	if (!vid)
		return -EOPNOTSUPP;

What prevents this condition from triggering in real life? The fact that
when a DSA_NOTIFIER_VLAN_ADD is emitted, it never targets a DSA link
directly. Instead, the notifier will always target either a user port or
a CPU port. DSA links just happen to get dragged in by:

static bool dsa_switch_vlan_match(struct dsa_switch *ds, int port,
				  struct dsa_notifier_vlan_info *info)
{
	...
	if (dsa_is_dsa_port(ds, port))
		return true;
	...
}

So for every DSA VLAN notifier, during the prepare phase, it will just
so happen that there will be somebody to say "no, don't do that".

This will become a problem when the switchdev prepare/commit transactional
model goes away. Every port needs to think on its own. DSA links can no
longer bluff and rely on the fact that the prepare phase will not go
through to the end, because there will be no prepare phase any longer.

Fix this issue before it becomes a problem, by having the "vid == 0"
check earlier than the check whether we are a CPU port / DSA link or not.
Also, the "vid == 0" check becomes unnecessary in the .port_vlan_add
callback, so we can remove it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
Changes in v4:
retouched the commit message a little bit.

Changes in v3:
Patch is new.

 drivers/net/dsa/mv88e6xxx/chip.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 4834be9e4e86..fb25cb87156a 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1535,13 +1535,13 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_vtu_entry vlan;
 	int i, err;

+	if (!vid)
+		return -EOPNOTSUPP;
+
 	/* DSA and CPU ports have to be members of multiple vlans */
 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
 		return 0;

-	if (!vid)
-		return -EOPNOTSUPP;
-
 	vlan.vid = vid - 1;
 	vlan.valid = false;

@@ -1920,9 +1920,6 @@ static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
 	struct mv88e6xxx_vtu_entry vlan;
 	int i, err;

-	if (!vid)
-		return -EOPNOTSUPP;
-
 	vlan.vid = vid - 1;
 	vlan.valid = false;

-- 
2.25.1