linux-kernel - Re: [PATCH net v1 1/1] net: phy: micrel: ksz8081: disable broadcast only if PHY address is not 0

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, 27 Jun 2024 15:56:42 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	stable@...r.kernel.org, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, Russell King <linux@...linux.org.uk>,
	netdev@...r.kernel.org, Lukasz Majewski <lukma@...x.de>
Subject: Re: [PATCH net v1 1/1] net: phy: micrel: ksz8081: disable broadcast
 only if PHY address is not 0

On Thu, Jun 27, 2024 at 07:33:53AM +0200, Oleksij Rempel wrote:
> Do not disable broadcast if we are using address 0 (broadcast) to
> communicate with this device. Otherwise we will use proper driver but no
> communication will be possible and no link changes will be detected.
> There are two scenarios where we can run in to this situation:
> - PHY is bootstrapped for address 0

What do you mean by bootstrapped to address 0? The strapping pins set
it to some other address, but the bootloader wrote to registers and
moved it to address 0? 

> - no PHY address is known and linux is scanning the MDIO bus, so first
>   respond and attached device will be on address 0.

So in this case, the PHY is really at address X, where X != 0. It
responds to all read requests, so the scanning finds it at all
addresses. It also stomps over other devices on the bus when scanning
for them, or probing them.

I'm not sure the current code is correct. But it is also going to be
messy to not break backwards compatibility for DT blobs say the device
is at address 0, when in fact it is not.

Is it possible to read the devices actual address from registers?

I'm wondering if probe should do something like:

int actual_address = phydev_read(phydev, 0x42);

if (actual_address == 0) {
	if (type->has_broadcast_disable) {
		phydev_dbg(phydev, "Disabling broadcast\n");
		kszphy_broadcast_disable(phydev);
	}

} else {
	if (actual_address != 0 &&
	 phydev->mdio.addr != actual_address &&
	 phydev->mdio.addr != 0) {
		if (type->has_broadcast_disable) {
			phydev_dbg(phydev, "Disabling broadcast\n");
			kszphy_broadcast_disable(phydev);
		}
        return -ENODEV;
	}
}

So if the devices really has an address is zero, turn off
broadcast. That will stop it stomping over other devices, but the
damage is probably already done in terms of scanning.

If the devices is really at some address other than 0, and we are
probing at a different address, and that address is not 0, turn off
broadcast and say the device does not exist. I think we need to
special case 0 because there are going to be some DT descriptions
which say the device is at 0, when in fact it is not. We might want to
add a phydev_warn() about this, to try to get the DT fixed.

> The fixes tag points to the latest refactoring, not to the initial point
> where kszphy_broadcast_disable() was introduced.
> 
> Fixes: 79e498a9c7da0 ("net: phy: micrel: Restore led_mode and clk_sel on resume")
> Cc: stable@...r.kernel.org
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>

Do you have a board which is going wrong because of this? Do you plan
to submit patches for earlier stable releases?

	Andrew