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]

Date:   Thu, 30 Sep 2021 11:40:43 +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 12:30:52PM +0200, Rafał Miłecki wrote:
> On 30.09.2021 12:17, Russell King (Oracle) wrote:
> > 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?)
> 
> I'll see if I can use buildroot to test unmodified kernel.
> 
> 
> > 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)
> > 
> 
> Unfortunately this doesn't seem to help

Hmm.

In phy_probe, can you add:

	WARN_ON(!(phydev->mdio.flags & MDIO_DEVICE_FLAG_PHY));

just to make sure we have a real PHY device there please? Maybe also
print the value of the flags argument.

MDIO_DEVICE_FLAG_PHY is set by phy_create_device() before the mutex is
initialised, so if it is set, the lock should be initialised.

Maybe also print mdiodev->flags in mdio_device_register() as well, so
we can see what is being registered and the flags being used for that
device.

Could it be that openwrt is carrying a patch that is causing this
issue?

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