netdev - Re: [PATCH net] net: dsa: Discard frames from unused ports

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:   Thu, 5 Apr 2018 04:17:57 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>
Subject: Re: [PATCH net] net: dsa: Discard frames from unused ports

On Wed, Apr 04, 2018 at 05:49:10PM -0700, Florian Fainelli wrote:
> On 04/04/2018 04:56 PM, Andrew Lunn wrote:
> > The Marvell switches under some conditions will pass a frame to the
> > host with the port being the CPU port. Such frames are invalid, and
> > should be dropped. Not dropping them can result in a crash when
> > incrementing the receive statistics for an invalid port.
> > 
> > Reported-by: Chris Healy <cphealy@...il.com>
> > Fixes: 5f6b4e14cada ("net: dsa: User per-cpu 64-bit statistics")
> 
> Are you sure this is the commit that introduced the problem?

Hi Florian

Well, the problem is it crashes when trying to update the
statistics. The CPU port is not allocated a p->stats64, only slave
ports get those. So before this patch, there was no crash and the
frame would be delivered to the master interface. This in itself is
probably not correct, but also not fatal. Talking to Chris, it seems
this behaviour has existing for a long while. I needed to use lldpd to
trigger the issue, because i assume the Marvell switch sees these as
special frames and forwards them to the CPU. The other thing is, the
code got refactored recently. So this fix will not rebase to too many
earlier versions. It needs a fix per tagging protocol for before the
common dsa_master_find_slave() was added.

> > -	return ds->ports[port].slave;
> > +	slave_port = &ds->ports[port];
> > +
> > +	if (slave_port->type != DSA_PORT_TYPE_USER)
> 
> Can we optimize this with an unlikely()?

Yes, that would make sense.

     Andrew