netdev - Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

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:   Wed, 11 Nov 2020 20:02:33 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Markus Blöchl <markus.bloechl@...tronik.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vladimir Oltean <olteanv@...il.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in
 promiscuous mode

On Wed, Nov 11, 2020 at 07:43:41AM -0800, Jakub Kicinski wrote:
> On Tue, 10 Nov 2020 16:39:58 +0100 Markus Blöchl wrote:
> > The rx-vlan-filter feature flag prevents unexpected tagged frames on
> > the wire from reaching the kernel in promiscuous mode.
> > Disable this offloading feature in the lan7800 controller whenever
> > IFF_PROMISC is set and make sure that the hardware features
> > are updated when IFF_PROMISC changes.
> > 
> > Signed-off-by: Markus Blöchl <markus.bloechl@...tronik.com>
> > ---
> > 
> > Notes:
> >     When sniffing ethernet using a LAN7800 ethernet controller, vlan-tagged
> >     frames are silently dropped by the controller due to the
> >     RFE_CTL_VLAN_FILTER flag being set by default since commit
> >     4a27327b156e("net: lan78xx: Add support for VLAN filtering.").
> > 
> >     In order to receive those tagged frames it is necessary to manually disable
> >     rx vlan filtering using ethtool ( `ethtool -K ethX rx-vlan-filter off` or
> >     corresponding ioctls ). Setting all bits in the vlan filter table to 1 is
> >     an even worse approach, imho.
> > 
> >     As a user I would probably expect that setting IFF_PROMISC should be enough
> >     in all cases to receive all valid traffic.
> >     Therefore I think this behaviour is a bug in the driver, since other
> >     drivers (notably ixgbe) automatically disable rx-vlan-filter when
> >     IFF_PROMISC is set. Please correct me if I am wrong here.
> 
> I've been mulling over this, I'm not 100% sure that disabling VLAN
> filters on promisc is indeed the right thing to do. The ixgbe doing
> this is somewhat convincing. OTOH users would not expect flow filters
> to get disabled when promisc is on, so why disable vlan filters?
> 
> Either way we should either document this as an expected behavior or
> make the core clear the features automatically rather than force
> drivers to worry about it.
> 
> Does anyone else have an opinion, please?

I agree it is weird, but it seems to be justified by the bridge code.
Note that the code was added around 2013, so it is possible it was
influenced by the behavior of existing drivers.

1. vi net/bridge/br_vlan.c +245

/* Add VLAN to the device filter if it is supported.
 * This ensures tagged traffic enters the bridge when
 * promiscuous mode is disabled by br_manage_promisc().
 */

2. The bridge device itself will not filter packets based on their VID
when it is in promiscuous mode:

vi net/bridge/br_input.c +45

vg = br_vlan_group_rcu(br);
/* Bridge is just like any other port.  Make sure the
 * packet is allowed except in promisc modue when someone
 * may be running packet capture.
 */
if (!(brdev->flags & IFF_PROMISC) &&
    !br_allowed_egress(vg, skb)) {
	kfree_skb(skb);
	return NET_RX_DROP;
}