netdev - Re: [PATCH net-next] net: bridge: pop vlan from skb if filtering is disabled but it's a 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 PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 12 Sep 2020 06:56:12 +0000
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     "stephen@...workplumber.org" <stephen@...workplumber.org>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Roopa Prabhu <roopa@...dia.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>
Subject: Re: [PATCH net-next] net: bridge: pop vlan from skb if filtering is
 disabled but it's a pvid

On Sat, 2020-09-12 at 02:16 +0300, Vladimir Oltean wrote:
> Currently the bridge untags VLANs from its VLAN group in
> __allowed_ingress() only when VLAN filtering is enabled.
> 
> When installing a pvid in egress-tagged mode, DSA switches have a
> problem:
> 
> ip link add dev br0 type bridge vlan_filtering 0
> ip link set swp0 master br0
> bridge vlan del dev swp0 vid 1
> bridge vlan add dev swp0 vid 1 pvid
> 
> When adding a VLAN on a DSA switch interface, DSA configures the VLAN
> membership of the CPU port using the same flags as swp0 (in this case
> "pvid and not untagged"), in an attempt to copy the frame as-is from
> ingress to the CPU.
> 
> However, in this case, the packet may arrive untagged on ingress, it
> will be pvid-tagged by the ingress port, and will be sent as
> egress-tagged towards the CPU. Otherwise stated, the CPU will see a VLAN
> tag where there was none to speak of on ingress.
> 
> When vlan_filtering is 1, this is not a problem, as stated in the first
> paragraph, because __allowed_ingress() will pop it. But currently, when
> vlan_filtering is 0 and we have such a VLAN configuration, we need an
> 8021q upper (br0.1) to be able to ping over that VLAN.
> 
> Make the 2 cases (vlan_filtering 0 and 1) behave the same way by popping
> the pvid, if the skb happens to be tagged with it, when vlan_filtering
> is 0.
> 
> There was an attempt to resolve this issue locally within the DSA
> receive data path, but even though we can determine that we are under a
> bridge with vlan_filtering=0, there are still some challenges:
> - we cannot be certain that the skb will end up in the software bridge's
>   data path, and for that reason, we may be popping the VLAN for
>   nothing. Example: there might exist an 8021q upper with the same VLAN,
>   or this interface might be a DSA master for another switch. In that
>   case, the VLAN should definitely not be popped even if it is equal to
>   the default_pvid of the bridge, because it will be consumed about the
>   DSA layer below.

Could you point me to a thread where these problems were discussed and why
they couldn't be resolved within DSA in detail ?

> - the bridge API only offers a race-free API for determining the pvid of
>   a port, br_vlan_get_pvid(), under RTNL.
> 

The API can be easily extended.

> And in fact this might not even be a situation unique to DSA. Any driver
> that receives untagged frames as pvid-tagged is now able to communicate
> without needing an 8021q upper for the pvid.
> 

I would prefer we don't add hardware/driver-specific fixes in the bridge, when
vlan filtering is disabled there should be no vlan manipulation/filtering done
by the bridge. This could potentially break users who have added 8021q devices
as bridge ports. At the very least this needs to be hidden behind a new option,
but I would like to find a way to actually push it back to DSA. But again adding
hardware/driver-specific options should be avoided.

Can you use tc to pop the vlan on ingress ? I mean the cases above are visible
to the user, so they might decide to add the ingress vlan rule.

Thanks,
 Nik

> Signed-off-by: Vladimir Oltean <olteanv@...il.com>
> Tested-by: Florian Fainelli <f.fainelli@...il.com>
> ---
>  net/bridge/br_vlan.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index d2b8737f9fc0..ecfdb9cd3183 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -580,7 +580,23 @@ bool br_allowed_ingress(const struct net_bridge *br,
>  	 * permitted.
>  	 */
>  	if (!br_opt_get(br, BROPT_VLAN_ENABLED)) {
> +		u16 v;
> +
>  		BR_INPUT_SKB_CB(skb)->vlan_filtered = false;
> +
> +		/* See comment in __allowed_ingress about how skb can end up
> +		 * here not having a hwaccel tag
> +		 */
> +		if (unlikely(!skb_vlan_tag_present(skb) &&
> +			     skb->protocol == br->vlan_proto)) {
> +			skb = skb_vlan_untag(skb);
> +			if (unlikely(!skb))
> +				return false;
> +		}
> +
> +		if (!br_vlan_get_tag(skb, &v) && v == br_get_pvid(vg))
> +			__vlan_hwaccel_clear_tag(skb);
> +
>  		return true;
>  	}
>