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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y/YPfxg8Ackb8zmW@shell.armlinux.org.uk>
Date:   Wed, 22 Feb 2023 12:50:07 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
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>, UNGLinuxDriver@...rochip.com,
        Vladimir Oltean <olteanv@...il.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

On Wed, Feb 22, 2023 at 04:17:38AM +0100, Marek Vasut wrote:
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 729b36eeb2c46..7fc2155d93d6e 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -319,7 +319,7 @@ static const u16 ksz8795_regs[] = {
>  	[S_BROADCAST_CTRL]		= 0x06,
>  	[S_MULTICAST_CTRL]		= 0x04,
>  	[P_XMII_CTRL_0]			= 0x06,
> -	[P_XMII_CTRL_1]			= 0x56,
> +	[P_XMII_CTRL_1]			= 0x06,

Looking at this driver, I have to say that it looks utterly vile
from the point of view of being sure that it is correct, and I
think this patch illustrates why.

You mention you're using a KSZ8794. This uses the ksz8795_regs
array, and ksz8_dev_ops. You claim this is about the P_GMII_1GBIT_M
bit, which is bit 6.

This bit is accessed only by ksz_get_gbit() and ksz_set_gbit().

Firstly, ksz_set_gbit() is only called from ksz_port_set_xmii_speed(),
which is only called from ksz9477_phylink_mac_link_up(). This is only
referenced by ksz9477_dev_ops and lan937x_dev_ops, but not ksz8_dev_ops.
Therefore, ksz_set_gbit() is not called for KSZ8794.

ksz_get_gbit() is only referenced by ksz9477.c in
ksz9477_get_interface(), called only by ksz9477_config_cpu_port().
This is only referenced by ksz9477_dev_ops, but not ksz8_dev_ops.

Therefore, my conclusion is that neither of the ksz_*_gbit()
functions are called on KSZ8794, and thus your change has no effect
on the driver's use of P_GMII_1GBIT_M - I think if you put some
debugging printk()s into both ksz_*_gbit() functions, it'll prove
that.

There's other places that P_XMII_CTRL_1 is accessed - ksz_set_xmii()
and ksz_get_xmii(). These look at the P_MII_SEL_M, P_RGMII_ID_IG_ENABLE
and P_RGMII_ID_EG_ENABLE bits - bits 0, 1, 3 and 4.

ksz_get_xmii() is only called by ksz9477_get_interface(), which we've
already looked at above as not being called.

ksz_set_xmii() is only called by ksz_phylink_mac_config(), which is
always called irrespective of the KSZ chip.

Now, let's look at functions that access P_XMII_CTRL_0. These are
ksz_set_100_10mbit() and ksz_duplex_flowctrl(). The former
accesses bit P_MII_100MBIT_M, which is bit 4. The latter looks at
bits 6, bit 5, and possibly bit 3 depending on the masks being used.
KSZ8795 uses ksz8795_masks, which omits bit 3, so bits 5 and 6.
Note... bit 6 is also P_GMII_1GBIT_M. So if ksz_duplex_flowctrl()
is ever called for the KSZ8795, then we have a situation where
the P_GMII_1GBIT_M will be manipulated.

ksz_set_100_10mbit() is only called from ksz_port_set_xmii_speed(),
which we've established won't be called.

ksz_duplex_flowctrl() is only called from ksz9477_phylink_mac_link_up()
which we've also established won't be called.

So, as far as I can see, P_XMII_CTRL_0 won't be accessed on this
device.

Now, what about other KSZ devices - I've analysed this for the KSZ8795,
but what about any of the others which use this register table? It
looks to me like those that use ksz8795_regs[] all use ksz8_dev_ops
and the same masks and bitvals, so they should be the same.

That is a hell of a lot of work to prove that setting both
P_XMII_CTRL_0 and P_XMII_CTRL_1 to point at the same register is
in fact safe. Given the number of registers, the masks, and bitval
arrays, doing this to prove every combination and then analysing
the code is utterly impractical - and thus why I label this driver
as "vile". Is there really no better option to these register
arrays, bitval arrays and mask arrays - something that makes it
easier to review and prove correctness?

I'm not going to give a reviewed-by for this, because... I could
have made a mistake in the above analysis given the vile nature
of this driver.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!