linux-kernel - Re: [PATCH 2/2] net: phy: Don't use drv when it is NULL in phy_attached

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] [day] [month] [year] [list]

Date:   Mon, 21 Aug 2017 15:16:04 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Romain Perier <romain.perier@...labora.com>
Cc:     Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] net: phy: Don't use drv when it is NULL in
 phy_attached_print

On Mon, Aug 21, 2017 at 09:52:35AM +0200, Romain Perier wrote:
> Currently, if this logging function is used prior the phy driver is
> binded to the phy device (that is usually done from .ndo_open),
> 'phydev->drv' might be NULL, resulting in a kernel crash. That is
> typically the case in the stmmac driver, info about the phy is displayed
> during the registration of the MDIO bus, and then genphy driver is binded
> to this phydev when .ndo_open is called.
> 
> This commit fixes the issue by using the right genphy driver, when
> phydev->drv is NULL.
> 
> Fixes: commit fbca164776e4 ("net: stmmac: Use the right logging functi")
> Signed-off-by: Romain Perier <romain.perier@...labora.com>
> ---
>  drivers/net/phy/phy_device.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 9493fb369682..b38926bc275f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -877,15 +877,24 @@ EXPORT_SYMBOL(phy_attached_info);
>  #define ATTACHED_FMT "attached PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)"
>  void phy_attached_print(struct phy_device *phydev, const char *fmt, ...)
>  {
> +	struct phy_driver *drv = phydev->drv;
> +
> +	if (!drv) {
> +		if (phydev->is_c45)
> +			drv = &genphy_10g_driver;
> +		else
> +			drv = &genphy_driver;
> +	}

Hi Romain

I don't like this. You end up with the same code twice. c45 does not
imply 10g, so i would not be surprised if sometime in the future this
changes. And then we have two places we need to make the same change.

I also wonder what happens if you load the PHY driver later, but
before it is bound. Will it then use the correct driver?


> +
>  	if (!fmt) {
>  		dev_info(&phydev->mdio.dev, ATTACHED_FMT "\n",
> -			 phydev->drv->name, phydev_name(phydev),
> +			 drv->name, phydev_name(phydev),

I would prefer (phydev->drv ? phydev->drv->name, "unknown")

  Andrew