netdev - Re: mv88e6240 configuration broken for B850v3

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: <20211206200111.3n4mtfz25fglhw4y@skbuf>
Date:   Mon, 6 Dec 2021 22:01:11 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Martyn Welch <martyn.welch@...labora.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, kernel@...labora.com,
        Russell King <rmk+kernel@...linux.org.uk>
Subject: Re: mv88e6240 configuration broken for B850v3

On Mon, Dec 06, 2021 at 08:53:46PM +0100, Andrew Lunn wrote:
> > > > Just out of curiosity, can you try this change? It looks like a
> > > > simple
> > > > case of mismatched conditions inside the mv88e6xxx driver between
> > > > what
> > > > is supposed to force the link down and what is supposed to force it
> > > > up:
> > > > 
> > > > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > > > index 20f183213cbc..d235270babf7 100644
> > > > --- a/net/dsa/port.c
> > > > +++ b/net/dsa/port.c
> > > > @@ -1221,7 +1221,7 @@ int dsa_port_link_register_of(struct dsa_port
> > > > *dp)
> > > >                 if (of_phy_is_fixed_link(dp->dn) || phy_np) {
> > > >                         if (ds->ops->phylink_mac_link_down)
> > > >                                 ds->ops->phylink_mac_link_down(ds, port,
> > > > -                                       MLO_AN_FIXED, PHY_INTERFACE_MODE_NA);
> > > > +                                       MLO_AN_PHY, PHY_INTERFACE_MODE_NA);
> > > >                         return dsa_port_phylink_register(dp);
> > > >                 }
> > > >                 return 0;
> > > 
> > > Yes, that appears to also make it work.
> > > 
> > > Martyn
> > 
> > Well, I just pointed out what the problem is, I don't know how to solve
> > it, honest! :)
> > 
> > It's clear that the code is wrong, because it's in an "if" block that
> > checks for "of_phy_is_fixed_link(dp->dn) || phy_np" but then it omits
> > the "phy_np" part of it. On the other hand we can't just go ahead and
> > say "if (phy_np) mode = MLO_AN_PHY; else mode = MLO_AN_FIXED;" because
> > MLO_AN_INBAND is also a valid option that we may be omitting. So we'd
> > have to duplicate part of the logic from phylink_parse_mode(), which
> > does not appear ideal at all. What would be ideal is if this fabricated
> > phylink call would not be done at all, but I don't know enough about the
> > systems that need it, I expect Andrew knows more.
> 
> phylink assumes interfaces start in the down state. It will then
> configure them and bring them up as needed. This is not always true
> with mv88e6xxx, the interface can already by up, and then the hardware
> and phylink have different state information. So this call was added
> to force the link down before phylink took control of it.
> 
> So i suspect something is missing when phylink sometime later does
> bring the interface up. It is not fully undoing what this down
> does. Maybe enable the dev_dbg() in mv88e6xxx_port_set_link() and see
> what value it has in both the good and bad case?

Andrew, here is mv88e6xxx_mac_link_down():

	if (((!mv88e6xxx_phy_is_internal(ds, port) &&
	      !mv88e6xxx_port_ppu_updates(chip, port)) ||
	     mode == MLO_AN_FIXED) && ops->port_sync_link)
		err = ops->port_sync_link(chip, port, mode, false);

and here is mv88e6xxx_mac_link_up():

	if ((!mv88e6xxx_phy_is_internal(ds, port) &&
	     !mv88e6xxx_port_ppu_updates(chip, port)) ||
	    mode == MLO_AN_FIXED) {
		(...)
		if (ops->port_sync_link)
			err = ops->port_sync_link(chip, port, mode, true);

This is the CPU port from Martyn's device tree:

	port@4 {
		reg = <4>;
		label = "cpu";
		ethernet = <&switch_nic>;
		phy-handle = <&switchphy4>;
	};

It has an internal PHY, so mv88e6xxx_phy_is_internal() will return true.
True negated is false, so the AND with the other PPU condition is always
false. BUT: the logic is: "force the link IF it doesn't have an internal
PHY OR it is a fixed link".

DSA fabricates a mv88e6xxx_mac_link_down call with MLO_AN_FIXED. So
->port_sync_link is called with false even if the PHY is internal, due
to the right hand operand to the || operator.

Then the real phylink, not the impersonator, comes along and calls
mv88e6xxx_mac_link_up with MLO_AN_PHY. The same check is now not
satisfied, because the input data has changed!

If we're going to impersonate phylink we could at least provide the same
arguments as phylink will.