netdev - Re: [PATCH net-next] net: phy: fix check 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200722144105.GB1339445@lunn.ch>
Date:   Wed, 22 Jul 2020 16:41:05 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, linux@...linux.org.uk,
        f.fainelli@...il.com, hkallweit1@...il.com, claudiu.manoil@....com,
        alexandru.marginean@....com, ioana.ciornei@....com,
        michael@...le.cc, colin.king@...onical.com
Subject: Re: [PATCH net-next] net: phy: fix check in get_phy_c45_ids

On Wed, Jul 22, 2020 at 02:52:09PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 20, 2020 at 08:26:54PM +0300, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@....com>
> > 
> > After the patch below, the iteration through the available MMDs is
> > completely short-circuited, and devs_in_pkg remains set to the initial
> > value of zero.
> > 
> > Due to devs_in_pkg being zero, the rest of get_phy_c45_ids() is
> > short-circuited too: the following loop never reaches below this point
> > either (it executes "continue" for every device in package, failing to
> > retrieve PHY ID for any of them):
> > 
> > 	/* Now probe Device Identifiers for each device present. */
> > 	for (i = 1; i < num_ids; i++) {
> > 		if (!(devs_in_pkg & (1 << i)))
> > 			continue;
> > 
> > So c45_ids->device_ids remains populated with zeroes. This causes an
> > Aquantia AQR412 PHY (same as any C45 PHY would, in fact) to be probed by
> > the Generic PHY driver.
> > 
> > The issue seems to be a case of submitting partially committed work (and
> > therefore testing something other than was submitted).
> > 
> > The intention of the patch was to delay exiting the loop until one more
> > condition is reached (the devs_in_pkg read from hardware is either 0, OR
> > mostly f's). So fix the patch to reflect that.
> > 
> > Tested with traffic on a LS1028A-QDS, the PHY is now probed correctly
> > using the Aquantia driver. The devs_in_pkg bit field is set to
> > 0xe000009a, and the MMDs that are present have the following IDs:
> > 
> > [    5.600772] libphy: get_phy_c45_ids: device_ids[1]=0x3a1b662
> > [    5.618781] libphy: get_phy_c45_ids: device_ids[3]=0x3a1b662
> > [    5.630797] libphy: get_phy_c45_ids: device_ids[4]=0x3a1b662
> > [    5.654535] libphy: get_phy_c45_ids: device_ids[7]=0x3a1b662
> > [    5.791723] libphy: get_phy_c45_ids: device_ids[29]=0x3a1b662
> > [    5.804050] libphy: get_phy_c45_ids: device_ids[30]=0x3a1b662
> > [    5.816375] libphy: get_phy_c45_ids: device_ids[31]=0x0
> > 
> > [    7.690237] mscc_felix 0000:00:00.5: PHY [0.5:00] driver [Aquantia AQR412] (irq=POLL)
> > [    7.704739] mscc_felix 0000:00:00.5: PHY [0.5:01] driver [Aquantia AQR412] (irq=POLL)
> > [    7.718918] mscc_felix 0000:00:00.5: PHY [0.5:02] driver [Aquantia AQR412] (irq=POLL)
> > [    7.733044] mscc_felix 0000:00:00.5: PHY [0.5:03] driver [Aquantia AQR412] (irq=POLL)
> > 
> > Fixes: bba238ed037c ("net: phy: continue searching for C45 MMDs even if first returned ffff:ffff")
> > Reported-by: Colin King <colin.king@...onical.com>
> > Reported-by: Ioana Ciornei <ioana.ciornei@....com>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > ---
> 
> This patch is repairing some pretty significant breakage. Could we
> please get some review before there start appearing user reports?
> 
> [ sorry for the breakage ]

I'm surprised it has not been merged, since the fix seems quite
obvious.

Reviewed-by: Andrew Lunn <andrew@...n.ch>

    Andrew