netdev - Re: [PATCH net-next] net: dsa: tag_dsa: Fix tx from VLAN uppers on non-filtering bridges

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: <20220308093653.c2enspat5mvah4n3@skbuf>
Date:   Tue, 8 Mar 2022 11:36:53 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     davem@...emloft.net, kuba@...nel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: tag_dsa: Fix tx from VLAN uppers on
 non-filtering bridges

On Mon, Mar 07, 2022 at 12:05:48PM +0100, Tobias Waldekranz wrote:
> In this situation (VLAN filtering disabled on br0):
> 
>     br0.10
>      /
>    br0
>    / \
> swp0 swp1
> 
> When a frame is transmitted from the VLAN upper, the bridge will send
> it down to one of the switch ports with forward offloading
> enabled. This will cause tag_dsa to generate a FORWARD tag. Before
> this change, that tag would have it's VID set to 10, even though VID
> 10 is not loaded in the VTU.
> 
> Before the blamed commit, the frame would trigger a VTU miss and be
> forwarded according to the PVT configuration. Now that all fabric
> ports are in 802.1Q secure mode, the frame is dropped instead.
> 
> Therefore, restrict the condition under which we rewrite an 802.1Q tag
> to a DSA tag. On standalone port's, reuse is always safe since we will
> always generate FROM_CPU tags in that case. For bridged ports though,
> we must ensure that VLAN filtering is enabled, which in turn
> guarantees that the VID in question is loaded into the VTU.
> 
> Fixes: d352b20f4174 ("net: dsa: mv88e6xxx: Improve multichip isolation of standalone ports")
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---
>  net/dsa/tag_dsa.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index c8b4bbd46191..e4b6e3f2a3db 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -127,6 +127,7 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  				   u8 extra)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	struct net_device *br_dev;
>  	u8 tag_dev, tag_port;
>  	enum dsa_cmd cmd;
>  	u8 *dsa_header;
> @@ -149,7 +150,16 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  		tag_port = dp->index;
>  	}
>  
> -	if (skb->protocol == htons(ETH_P_8021Q)) {
> +	br_dev = dsa_port_bridge_dev_get(dp);
> +
> +	/* If frame is already 802.1Q tagged, we can convert it to a DSA
> +	 * tag (avoiding a memmove), but only if the port is standalone
> +	 * (in which case we always send FROM_CPU) or if the port's
> +	 * bridge has VLAN filtering enabled (in which case the CPU port
> +	 * will be a member of the VLAN).
> +	 */
> +	if (skb->protocol == htons(ETH_P_8021Q) &&
> +	    (!br_dev || br_vlan_enabled(br_dev))) {

Conservative patch. If !br_dev, we could/should inject using
MV88E6XXX_VID_STANDALONE. But since we use FROM_CPU, the classified VLAN
probably does not make a difference that I can see, so there is no
reason to change this now (and certainly not in the same patch).

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

>  		if (extra) {
>  			skb_push(skb, extra);
>  			dsa_alloc_etype_header(skb, extra);
> @@ -166,10 +176,9 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  			dsa_header[2] &= ~0x10;
>  		}
>  	} else {
> -		struct net_device *br = dsa_port_bridge_dev_get(dp);
>  		u16 vid;
>  
> -		vid = br ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE;
> +		vid = br_dev ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE;
>  
>  		skb_push(skb, DSA_HLEN + extra);
>  		dsa_alloc_etype_header(skb, DSA_HLEN + extra);
> -- 
> 2.25.1
>