netdev - Re: [PATCH net 1/2] net: mscc: ocelot: fix vlan_filtering when enslaving to bridge before link is up

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: <20191026230318.GE3125@piout.net>
Date:   Sun, 27 Oct 2019 01:03:18 +0200
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        joergen.andreasen@...rochip.com, allan.nielsen@...rochip.com,
        antoine.tenart@...tlin.com, f.fainelli@...il.com,
        vivien.didelot@...il.com, andrew@...n.ch
Subject: Re: [PATCH net 1/2] net: mscc: ocelot: fix vlan_filtering when
 enslaving to bridge before link is up

On 26/10/2019 21:04:26+0300, Vladimir Oltean wrote:
> Background information: the driver operates the hardware in a mode where
> a single VLAN can be transmitted as untagged on a particular egress
> port. That is the "native VLAN on trunk port" use case. Its value is
> held in port->vid.
> 
> Consider the following command sequence (no network manager, all
> interfaces are down, debugging prints added by me):
> 
> $ ip link add dev br0 type bridge vlan_filtering 1
> $ ip link set dev swp0 master br0
> 
> Kernel code path during last command:
> 
> br_add_slave -> ocelot_netdevice_port_event (NETDEV_CHANGEUPPER):
> [   21.401901] ocelot_vlan_port_apply: port 0 vlan aware 0 pvid 0 vid 0
> 
> br_add_slave -> nbp_vlan_init -> switchdev_port_attr_set -> ocelot_port_attr_set (SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING):
> [   21.413335] ocelot_vlan_port_apply: port 0 vlan aware 1 pvid 0 vid 0
> 
> br_add_slave -> nbp_vlan_init -> nbp_vlan_add -> br_switchdev_port_vlan_add -> switchdev_port_obj_add -> ocelot_port_obj_add -> ocelot_vlan_vid_add
> [   21.667421] ocelot_vlan_port_apply: port 0 vlan aware 1 pvid 1 vid 1
> 
> So far so good. The bridge has replaced the driver's default pvid used
> in standalone mode (0) with its own default_pvid (1). The port's vid
> (native VLAN) has also changed from 0 to 1.
> 
> $ ip link set dev swp0 up
> 
> [   31.722956] 8021q: adding VLAN 0 to HW filter on device swp0
> do_setlink -> dev_change_flags -> vlan_vid_add -> ocelot_vlan_rx_add_vid -> ocelot_vlan_vid_add:
> [   31.728700] ocelot_vlan_port_apply: port 0 vlan aware 1 pvid 1 vid 0
> 
> The 8021q module uses the .ndo_vlan_rx_add_vid API on .ndo_open to make
> ports be able to transmit and receive 802.1p-tagged traffic by default.
> This API is supposed to offload a VLAN sub-interface, which for a switch
> port means to add a VLAN that is not a pvid, and tagged on egress.
> 
> But the driver implementation of .ndo_vlan_rx_add_vid is wrong: it adds
> back vid 0 as "egress untagged". Now back to the initial paragraph:
> there is a single untagged VID that the driver keeps track of, and that
> has just changed from 1 (the pvid) to 0. So this breaks the bridge
> core's expectation, because it has changed vid 1 from untagged to
> tagged, when what the user sees is.
> 
> $ bridge vlan
> port    vlan ids
> swp0     1 PVID Egress Untagged
> 
> br0      1 PVID Egress Untagged
> 
> But curiously, instead of manifesting itself as "untagged and
> pvid-tagged traffic gets sent as tagged on egress", the bug:
> 
> - is hidden when vlan_filtering=0
> - manifests as dropped traffic when vlan_filtering=1, due to this setting:
> 
> 	if (port->vlan_aware && !port->vid)
> 		/* If port is vlan-aware and tagged, drop untagged and priority
> 		 * tagged frames.
> 		 */
> 		val |= ANA_PORT_DROP_CFG_DROP_UNTAGGED_ENA |
> 		       ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
> 		       ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA;
> 
> which would have made sense if it weren't for this bug. The setting's
> intention was "this is a trunk port with no native VLAN, so don't accept
> untagged traffic". So the driver was never expecting to set VLAN 0 as
> the value of the native VLAN, 0 was just encoding for "invalid".
> 
> So the fix is to not send 802.1p traffic as untagged, because that would
> change the port's native vlan to 0, unbeknownst to the bridge, and
> trigger unexpected code paths in the driver.
> 
> Cc: Antoine Tenart <antoine.tenart@...tlin.com>
> Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>
> Fixes: 7142529f1688 ("net: mscc: ocelot: add VLAN filtering")
> Signed-off-by: Vladimir Oltean <olteanv@...il.com>
Acked-by: Alexandre Belloni <alexandre.belloni@...tlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 7190fe4c1095..552252331e55 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -915,7 +915,7 @@ static int ocelot_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
>  static int ocelot_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>  				  u16 vid)
>  {
> -	return ocelot_vlan_vid_add(dev, vid, false, true);
> +	return ocelot_vlan_vid_add(dev, vid, false, false);
>  }
>  
>  static int ocelot_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
> -- 
> 2.17.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com