netdev - RE: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

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]

Message-ID: <AM4PR04MB160469BF592328331647BB73EC870@AM4PR04MB1604.eurprd04.prod.outlook.com>
Date:   Fri, 9 Dec 2016 09:09:14 +0000
From:   Madalin-Cristian Bucur <madalin.bucur@....com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Johan Hovold <johan@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "rmk+kernel@....linux.org.uk" <rmk+kernel@....linux.org.uk>,
        "andrew@...n.ch" <andrew@...n.ch>
Subject: RE: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a
 different owner

> From: netdev-owner@...r.kernel.org On Behalf Of Florian Fainelli
> Sent: Thursday, December 08, 2016 7:54 PM
> To: Johan Hovold <johan@...nel.org>
> On 12/08/2016 09:01 AM, Johan Hovold wrote:
> > On Thu, Dec 08, 2016 at 08:47:54AM -0800, Florian Fainelli wrote:
> >> On 12/08/2016 08:27 AM, Johan Hovold wrote:
> >>> On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:
> >>>> Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way
> we
> >>>> dealt with MDIO bus module reference count, but sort of introduced a
> >>>> regression in that, if an Ethernet driver registers its own MDIO bus
> >>>> driver, as is common, we will end up with the Ethernet driver's
> >>>> module->refnct set to 1, thus preventing this driver from any
> removal.
> >>>>
> >>>> Fix this by comparing the network device's device driver owner
> against
> >>>> the MDIO bus driver owner, and only if they are different, increment
> the
> >>>> MDIO bus module refcount.
> >>>>
> >>>> Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
> >>>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> >>>> ---
> >>>> Russell,
> >>>>
> >>>> I verified this against the ethoc driver primarily (on a TS7300
> board)
> >>>> and bcmgenet.
> >>>>
> >>>> Thanks!
> >>>>
> >>>>  drivers/net/phy/phy_device.c | 16 +++++++++++++---
> >>>>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/phy/phy_device.c
> b/drivers/net/phy/phy_device.c
> >>>> index 1a4bf8acad78..c4ceb082e970 100644
> >>>> --- a/drivers/net/phy/phy_device.c
> >>>> +++ b/drivers/net/phy/phy_device.c
> >>>> @@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
> >>>>  int phy_attach_direct(struct net_device *dev, struct phy_device
> *phydev,
> >>>>  		      u32 flags, phy_interface_t interface)
> >>>>  {
> >>>> +	struct module *ndev_owner = dev->dev.parent->driver->owner;
> >>>
> >>> Is this really safe? A driver does not need to set a parent device,
> and
> >>> in that case you get a NULL-deref here (I tried using cpsw).
> >>
> >> Humm, cpsw does call SET_NETDEV_DEV() which should take care of that,
> is
> >> the call made too late? Do you have an example oops?
> >
> > Sorry if I was being unclear, cpsw does set a parent device, but there
> > are network driver that do not. Perhaps such drivers will never hit this
> > code path, but I can't say for sure and everything appear to work for
> > cpsw if you comment out that SET_NETDEV_DEV (well, at least before this
> > patch).
> 
> You were clear, I did not understand that you exercised this with cpsw
> to see whether this was safe in all conditions.
> 
> >
> >> I don't mind safeguarding this with a check against dev->dev.parent,
> but
> >> I would like to fix the drivers where relevant too, since
> >> SET_NETDEV_DEV() should really be called, otherwise a number of things
> >> just don't work
> >
> > I grepped for for register_netdev and think I saw a number of drivers
> > which do not call SET_NETDEV_DEV.
> >
> > Again, perhaps they will never hit this path, but thought I should ask.
> 
> You are absolutely right, this is a potential problem, so far I found
> two legitimate drivers that do not call SET_NETDEV_DEV (lantiq_etop.c
> and cpmac.c, both fixed), and Freescale's FMAN driver, which I have a
> hard time understanding what it does with mac_dev->net_dev...
> 
> Thanks!
> --
> Florian

Hi Florian,

The Freescale DPAA Ethernet driver is in drivers/net/ethernet/freescale/dpaa:

drivers/net/ethernet/freescale/dpaa/dpaa_eth.c:2501:	SET_NETDEV_DEV(net_dev, dev);

and it is making use of the MAC and ports driver in the FMan driver (and of the
QBMan drivers in drivers/soc/fsl/qbman but that's off topic). You need to look
at the net-next tree for this, the drivers were gradually added.

Madalin