netdev - Re: [RFC PATCH net 2/4] net: dsa: prevent hardware forwarding between unbridged 8021q uppers

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] [day] [month] [year] [list]

Message-ID: <87k0qgp35g.fsf@waldekranz.com>
Date:   Tue, 09 Mar 2021 19:56:11 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "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>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [RFC PATCH net 2/4] net: dsa: prevent hardware forwarding between unbridged 8021q uppers

On Tue, Mar 09, 2021 at 04:16, Vladimir Oltean <olteanv@...il.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> Tobias reports that the following set of commands, which bridge two
> ports that have 8021q uppers with the same VID, is incorrectly accepted
> by DSA as valid:
>
> .100  br0  .100
>    \  / \  /
>    lan0 lan1
>
> ip link add dev br0 type bridge vlan_filtering 1
> ip link add dev lan0.100 link lan0 type vlan id 100
> ip link add dev lan1.100 link lan1 type vlan id 100

If I move this line...

> ip link set dev lan0 master br0
> ip link set dev lan1 master br0 # This should fail but doesn't

...down here, the config is (erroneously) accepted.

> Again, this is a variation of the same theme of 'all VLANs kinda smell
> the same in hardware, you can't tell if they came from 8021q or from the
> bridge'. When the base interfaces are bridged, the expectation of the
> Linux network stack is that traffic received by other upper interfaces
> except the bridge is not captured by the bridge rx_handler, therefore
> not subject to forwarding. So the above setup should not do forwarding
> for VLAN ID 100, but it does it nonetheless. So it should be denied.
>
> Reported-by: Tobias Waldekranz <tobias@...dekranz.com>
> Fixes: 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---

This is what I meant by having bits and pieces of this validation
scattered in multiple places, some things being checked for certain
events but not for others, etc.

I took an initial stab at this to show what I mean:

https://lore.kernel.org/netdev/20210309184244.1970173-1-tobias@waldekranz.com

I am sure there are holes in this as well, hence RFC, but I think it
will be much easier to make sure that we avoid ordering issues using a
structure like this.

What do you think?