linux-kernel - Re: [PATCH net-next] net: dsa: mt7530: drop untagged frames on VLAN-aware ports without PVID

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]

Date:   Fri, 6 Aug 2021 03:17:40 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     DENG Qingfang <dqfext@...il.com>
Cc:     Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: mt7530: drop untagged frames on
 VLAN-aware ports without PVID

On Fri, Aug 06, 2021 at 01:23:14AM +0800, DENG Qingfang wrote:
> @@ -1624,11 +1631,26 @@ mt7530_port_vlan_add(struct dsa_switch *ds, int port,
>  	if (pvid) {
>  		priv->ports[port].pvid = vlan->vid;
>  
> +		/* Accept all frames if PVID is set */
> +		mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
> +			   MT7530_VLAN_ACC_ALL);
> +
>  		/* Only configure PVID if VLAN filtering is enabled */
>  		if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
>  			mt7530_rmw(priv, MT7530_PPBV1_P(port),
>  				   G0_PORT_VID_MASK,
>  				   G0_PORT_VID(vlan->vid));
> +	} else if (priv->ports[port].pvid == vlan->vid) {
> +		/* This VLAN is overwritten without PVID, so unset it */
> +		priv->ports[port].pvid = G0_PORT_VID_DEF;
> +
> +		/* Only accept tagged frames if the port is VLAN-aware */
> +		if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
> +			mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
> +				   MT7530_VLAN_ACC_TAGGED);
> +
> +		mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
> +			   G0_PORT_VID_DEF);
>  	}
>  
>  	mutex_unlock(&priv->reg_mutex);

Good catch with this condition, sja1105 and ocelot are buggy in this
regard, it seems, probably others too. Need to fix them. Although
honestly I would probably rather spend the time patching the bridge
already to not accept duplicate VLAN entries from user space, just with
different flags, it's just too complex to handle the overwrites everywhere...
Plus, bridge accepting duplicate VLANs means we cannot refcount them on
DSA and CPU ports at the cross-chip level, which in turn means we can
never delete them from those ports.

Anyhow, enough rambling.

Reviewed-by: Vladimir Oltean <olteanv@...il.com>