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: <955416fe-4da4-b1ec-aadb-9b816f02d7f2@gmail.com>
Date:   Thu, 30 Sep 2021 14:51:40 +0200
From:   Rafał Miłecki <zajec5@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
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 30.09.2021 14:30, Russell King (Oracle) wrote:
>> It's actually OpenWrt's downstream swconfig-based b53 driver that
>> matches this device.
>>
>> I'm confused as downstream b53_mdio.c calls phy_driver_register(). Why
>> does it match MDIO device then? I thought MDIO devices should be
>> matches only with drivers using mdio_driver_register().
> 
> Note that I've no idea what he swconfig-based b53 driver looks like,
> I don't have the source for that to hand.
> 
> If it calls phy_driver_register(), then it is registering a driver for
> a MDIO device wrapped in a struct phy_device. If this driver has a
> .of_match_table member set, then this is wrong - the basic rule is
> 
> 	PHY drivers must never match using DT compatibles.
> 
> because this is exactly what will occur - it bypasses the check that
> the mdio_device being matched is in fact wrapped by a struct phy_device,
> and we will access members of the non-existent phy_device, including
> the "uninitialised" mutex.
> 
> If the swconfig-based b53 driver does want to bind to a phy_device based
> DT node, then it needs to match using either a custom .match_phy_device
> method in the PHY driver, or it needs to match using the PHY IDs.

Sorry, I should have linked downstream b53_mdio.c . It's:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c;h=98cdbffe73c7354f4401389dfcc96014bff62588;hb=HEAD

You can see that is *uses* of_match_table.

What about refusing bugged drivers like above b53 with something like:

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index b848439fa..76c8197d3 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -941,6 +941,10 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
  {
         struct mdio_device *mdio = to_mdio_device(dev);

+       if (WARN_ONCE(!(mdio->flags & MDIO_DEVICE_FLAG_PHY),
+                "Bugged driver %s\n", drv->name))
+               return 0;
+
         if (of_driver_match_device(dev, drv))
                 return 1;