Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 10 May 2019 22:04:05 +0100
From:   Russell King - ARM Linux admin <linux@armlinux.org.uk>
To:     Vicente Bergas <vicencb@gmail.com>
Cc:     Serge Semin <fancer.lancer@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: net: phy: realtek: regression, kernel null pointer dereference
Message-ID: <20190510210405.tehgan2s5rhimihc@shell.armlinux.org.uk>
References: <16f75ff4-e3e3-4d96-b084-e772e3ce1c2b@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <16f75ff4-e3e3-4d96-b084-e772e3ce1c2b@gmail.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, May 10, 2019 at 05:05:13PM +0200, Vicente Bergas wrote:
> Hello,
> there is a regression on linux v5.1-9573-gb970afcfcabd with a kernel null
> pointer dereference.
> The issue is the commit f81dadbcf7fd067baf184b63c179fc392bdb226e
>  net: phy: realtek: Add rtl8211e rx/tx delays config
> which uncovered a bug in phy-core when attempting to call
>  phydev->drv->read_page
> which can be null.
> The patch to drivers/net/phy/phy-core.c below fixes the kernel null pointer
> dereference. After applying the patch, there is still no network. I have
> also tested the patch to drivers/net/phy/realtek.c, but no success. The
> system hangs forever while initializing eth0.

You're not supposed to call these functions unless you provide the page
read/write page functions.  The fact that this code has crept in shows
that the patch adding the call to phy_select_page() in the realtek
driver was patently never tested, which, IMHO is bad software
engineering practice.  No, it's not even engineering practice, it's
an untested hack.

I don't see any point in adding run-time checks - that will only add
additional code, and we lose the backtrace.  The resulting oops from
trying to use these will give a backtrace and show exactly where the
problem is, including which driver is at fault.

The answer is... fix the driver to provide the required functions
before attempting to use an interface that requires said functions!

> 
> Any suggestions?
> 
> Regards,
>  Vicen?.
> 
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -648,11 +648,17 @@
> 
> static int __phy_read_page(struct phy_device *phydev)
> {
> +	if (!phydev->drv->read_page)
> +		return -EOPNOTSUPP;
> +	
> 	return phydev->drv->read_page(phydev);
> }
> 
> static int __phy_write_page(struct phy_device *phydev, int page)
> {
> +	if (!phydev->drv->write_page)
> +		return -EOPNOTSUPP;
> +
> 	return phydev->drv->write_page(phydev, page);
> }
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -214,8 +214,10 @@
> 	 * for details).
> 	 */
> 	oldpage = phy_select_page(phydev, 0x7);
> -	if (oldpage < 0)
> -		goto err_restore_page;
> +	if (oldpage < 0) {
> +		dev_warn(&phydev->mdio.dev, "Unable to set rgmii delays\n");
> +		return 0;
> +	}
> 
> 	ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
> 	if (ret)
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up