netdev - Re: Lockup in phy_probe() for MDIO device (Broadcom's switch)

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: <YVWOp/2Nj/E1dpe3@shell.armlinux.org.uk>
Date:   Thu, 30 Sep 2021 11:17:11 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Rafał Miłecki <zajec5@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        BCM Kernel Feedback <bcm-kernel-feedback-list@...adcom.com>,
        Vivek Unune <npcomplete13@...il.com>
Subject: Re: Lockup in phy_probe() for MDIO device (Broadcom's switch)

On Thu, Sep 30, 2021 at 11:58:21AM +0200, Rafał Miłecki wrote:
> This isn't necessarily a PHY / MDIO regression. It could be some core
> change that exposed a PHY / MDIO bug.

I think what's going on is that the switch device is somehow being
probed by phylib. It looks to me like we don't check that the mdio
device being matched in phy_bus_match() is actually a PHY (by
checking whether mdiodev->flags & MDIO_DEVICE_FLAG_PHY is true
before proceeding with any matching.)

We do, however, check the driver side. This looks to me like a problem
especially when the mdio bus can contain a mixture of PHY devices and
non-PHY devices. However, I would expect this to also be blowing up in
the mainline kernel as well - but it doesn't seem to.

Maybe Andrew can provide a reason why this doesn't happen - maybe we've
just been lucky with out-of-bounds read accesses (to the non-existent
phy_device wrapped around the mdio_device?)

If my theory is correct, this patch should solve your issue:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ba5ad86ec826..dac017174ab1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -462,7 +462,8 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
 	const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids);
 	int i;
 
-	if (!(phydrv->mdiodrv.flags & MDIO_DEVICE_IS_PHY))
+	if (!(phydrv->mdiodrv.flags & MDIO_DEVICE_IS_PHY) ||
+	    !(phydev->mdio.flags & MDIO_DEVICE_FLAG_PHY))
 		return 0;
 
 	if (phydrv->match_phy_device)

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