netdev - Re: [RFC PATCH net-next 07/11] net: dsa: microchip: lan937x: update switch register

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:   Sat, 4 Feb 2023 01:37:17 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Rakesh Sankaranarayanan <rakesh.sankaranarayanan@...rochip.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        f.fainelli@...il.com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, woojung.huh@...rochip.com,
        UNGLinuxDriver@...rochip.com, linux@...linux.org.uk
Subject: Re: [RFC PATCH net-next 07/11] net: dsa: microchip: lan937x: update
 switch register

On Thu, Feb 02, 2023 at 04:40:36PM +0100, Andrew Lunn wrote:
> On Thu, Feb 02, 2023 at 06:29:26PM +0530, Rakesh Sankaranarayanan wrote:
> > Second switch in cascaded connection doesn't have port with macb
> > interface. dsa_switch_register returns error if macb interface is
> > not up. Due to this reason, second switch in cascaded connection will
> > not report error during dsa_switch_register and mib thread work will be
> > invoked even if actual switch register is not done. This will lead to
> > kernel warning and it can be avoided by checking device tree setup
> > status. This will return true only after actual switch register is done.
> 
> What i think you need to do is move the code into ksz_setup().
> 
> With a D in DSA setup, dsa_switch_register() adds the switch to the
> list of switches, and then a check is performed to see if all switches
> in the cluster have been registered. If not, it just returns. If all
> switches have been registered, it then iterates over all the switches
> can calls dsa_switch_ops.setup().
> 
> By moving the start of the MIB counter into setup(), it will only be
> started once all the switches are present, and it means you don't need
> to look at DSA core internal state.

+1

Also there's a bug in its own right in ksz_mib_read_work(), here:

			if (!netif_carrier_ok(dp->slave))
				mib->cnt_ptr = dev->info->reg_mib_cnt;

The code accesses dp->slave, so naturally it kicks the bucket for
cascade ports.

It doesn't crash with CPU ports because dp->slave is in a union with
dp->master, which is also a struct net_device * with its own carrier:

struct dsa_port {
	/* A CPU port is physically connected to a master device.
	 * A user port exposed to userspace has a slave device.
	 */
	union {
		struct net_device *master;
		struct net_device *slave;
	};

This needs to be fixed, since accessing the DSA master through a
dp->slave pointer is dangerous and unintended.

Easiest thing to do would be to only check link state if (dsa_port_is_user(dp)).
For other ports always read all MIB counters.