netdev - Re: [PATCH] net: dsa: microchip: Fix gigabit set and get function for KSZ87xx

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: <20230222210853.pilycwhhwmf7csku@skbuf>
Date:   Wed, 22 Feb 2023 23:08:53 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Marek Vasut <marex@...x.de>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        UNGLinuxDriver@...rochip.com,
        Woojung Huh <woojung.huh@...rochip.com>, stable@...r.kernel.org
Subject: Re: [PATCH] net: dsa: microchip: Fix gigabit set and get function
 for KSZ87xx

Please summarize in the commit title what is the user-visible impact of
the problem that is being fixed. Short and to the point.

On Wed, Feb 22, 2023 at 04:17:38AM +0100, Marek Vasut wrote:
> Per KSZ8794 [1] datasheet DS00002134D page 54 TABLE 4-4: PORT REGISTERS,
> it is Register 86 (0x56): Port 4 Interface Control 6 which contains the
> Is_1Gbps field.

Good thing you mention Is_1Gbps (even though it's irrelevant to the
change you're proposing, since ksz_port_set_xmii_speed() is only called
by ksz9477_phylink_mac_link_up()).

That is actually what I want to bring up. If you change the speed in
your fixed-link nodes (CPU port and DSA master) to 100 Mbps on KSZ87xx,
does it work? No, right? Because P_GMII_1GBIT_M always remains at its
hardware default value, which is selected based on pin strapping.
That's a bug, and should be fixed too.

Good thing you brought this up, I wouldn't have mentioned it if it
wasn't in the commit message.

> Currently, the driver uses PORT read function on register P_XMII_CTRL_1
> to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit.

Provably false. The driver does do that, but not for KSZ87xx.
Please delete red herrings from the commit message, they do not help
assess users if they care about backporting a patch to a custom tree
or not.

> The problem is, the register P_XMII_CTRL_1 address is already 0x56,
> which is the converted PORT register address instead of the offset
> within PORT register space that PORT read function expects and
> converts into the PORT register address internally. The incorrectly
> double-converted register address becomes 0xa6, which is what the PORT
> read function ultimatelly accesses, and which is a non-existent
                ~~~~~~~~~~~
                ultimately

> register on the KSZ8794/KSZ8795 .
> 
> The correct value for P_XMII_CTRL_1 is 0x6, which gets converted into
> port address 0x56, which is Register 86 (0x56): Port 4 Interface Control 6
> per KSZ8794 datasheet, i.e. the correct register address.
> 
> To make this worse, there are multiple other call sites which read and
                                ~~~~~~~~
                                multiple implies more than 1.

There is no call site other than ksz_set_xmii(). Please delete false
information from the commit message.

> even write the P_XMII_CTRL_1 register, one of them is ksz_set_xmii(),
> which is responsible for configuration of RGMII delays. These delays
> are incorrectly configured and a non-existent register is written
> without this change.

Not only RGMII delays, but also P_MII_SEL_M (interface mode selection).

The implication of writing the value at an undocumented address is that
the real register 0x56 remains with the value decided by pin strapping
(which may or may not be adequate for Linux runtime). This is absolutely
the same class of bug as what happens with Is_1Gbps.

> Fix the P_XMII_CTRL_1 register offset to resolve these problems.
> 
> [1] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8794CNX-Data-Sheet-DS00002134.pdf
> 
> Fixes: 46f80fa8981b ("net: dsa: microchip: add common gigabit set and get function")

Technically, the problem was introduced by:

Fixes: c476bede4b0f ("net: dsa: microchip: ksz8795: use common xmii function")

because that's when ksz87xx was transitioned from the old logic (which
also used to set Is_1Gbps) to the new one.

And that same commit is also to blame for the Is_1Gbps bug, because the
new logic from ksz8795_cpu_interface_select() should have called not
only ksz_set_xmii(), but also ksz_set_gbit() for code-wise identical
behavior. It didn't do that. Then with commit f3d890f5f90e ("net: dsa:
microchip: add support for phylink mac config"), this incomplete
configuration just got moved around.

> Signed-off-by: Marek Vasut <marex@...x.de>

The contents of the patch is not wrong, but the commit message that
describes it misses a lot of points which make non-zero difference to
someone trying to assess whether a patch fixes a problem he's seeing or not.

Even worse, there is another _actual_ Is_1Gbps bug which I've presented
above, which this patch does *not* fix.