netdev - RE: [PATCH] net: phy: fix a bug in get_phy_c45

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: <BLUPR03MB14758A73606C88602401C3BFE22C0@BLUPR03MB1475.namprd03.prod.outlook.com>
Date:	Mon, 2 Nov 2015 11:00:15 +0000
From:	Shaohui Xie <Shaohui.Xie@...escale.com>
To:	David Miller <davem@...emloft.net>,
	"shh.xie@...il.com" <shh.xie@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"f.fainelli@...il.com" <f.fainelli@...il.com>
Subject: RE: [PATCH] net: phy: fix a bug in get_phy_c45_ids

> -----Original Message-----
> From: David Miller [mailto:davem@...emloft.net]
> Sent: Monday, November 02, 2015 1:14 AM
> To: shh.xie@...il.com
> Cc: netdev@...r.kernel.org; f.fainelli@...il.com; Xie Shaohui-B21989
> Subject: Re: [PATCH] net: phy: fix a bug in get_phy_c45_ids
> 
> From: <shh.xie@...il.com>
> Date: Thu, 29 Oct 2015 17:09:37 +0800
> 
> > From: Shaohui Xie <Shaohui.Xie@...escale.com>
> >
> > When probing devices-in-package for a c45 phy, device zero is the last
> > device to probe, in a rare situation which driver can read a '0' from
> > the device zero, thus c45_ids->devices_in_package is set to '0', so
> > the loop condition of probing is matched, see codes below:
> >
> > for (i = 1;i < num_ids && c45_ids->devices_in_package == 0;i++)
> >
> > driver will run in a dead loop.
> >
> > So after probing the device zero, driver should stop the probing loop.
> >
> > Signed-off-by: Shaohui Xie <Shaohui.Xie@...escale.com>
> 
> This bug only exists because the loop is extremely confusing.
> 
> Please fix this by restructuring the loop:
> 
> 1) Break out this code:
> 
> 		reg_addr = MII_ADDR_C45 | i << 16 | MDIO_DEVS2;
> 		phy_reg = mdiobus_read(bus, addr, reg_addr);
> 		if (phy_reg < 0)
> 			return -EIO;
> 		c45_ids->devices_in_package = (phy_reg & 0xffff) << 16;
> 
> 		reg_addr = MII_ADDR_C45 | i << 16 | MDIO_DEVS1;
> 		phy_reg = mdiobus_read(bus, addr, reg_addr);
> 		if (phy_reg < 0)
> 			return -EIO;
> 		c45_ids->devices_in_package |= (phy_reg & 0xffff);
> 
>    into a helper function that takes "c45_ids, bus, addr, i" as arguments
>    and returns an error, either 0 or -EIO.  Call it
> "phy_check_devs_in_pkg"
>    or similar.
> 
> 2) Rewrite the loop as:
> 
> 	for (i = 1;
> 	     i < num_ids && c45_ids->devices_in_package == 0;
> 	     i++) {
> 		err = phy_check_devs_in_pkg(c45_ids, bus, addr, i);
> 		if (err < 0)
> 			return err;
> 		if ((c45_ids->devices_in_package & 0x1fffffff) == 0x1fffffff)
> {
> 			if (i) {
> 				/*  If mostly Fs, there is no device there,
> 				 *  then let's continue to probe more, as some
> 				 *  10G PHYs have zero Devices In package,
> 				 *  e.g. Cortina CS4315/CS4340 PHY.
> 				 */
> 				err = phy_check_devs_in_pkg(c45_ids, bus, addr,
> 0);
> 				if (err)
> 					return err;
> 				break;
> 			}
> 		} else {
> 				/* no device there, let's get out of here */
> 				*phy_id = 0xffffffff;
> 				return 0;
> 			}
> 		}
> 	}
I've addressed the above in V2.

Thank you!

--Shaohui

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html