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]

Date:   Wed, 22 Jul 2020 14:52:09 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     davem@...emloft.net, netdev@...r.kernel.org
Cc:     linux@...linux.org.uk, f.fainelli@...il.com, andrew@...n.ch,
        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 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 ]

>  drivers/net/phy/phy_device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 49e98a092b96..1b9523595839 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -734,8 +734,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
>  	/* Find first non-zero Devices In package. Device zero is reserved
>  	 * for 802.3 c45 complied PHYs, so don't probe it at first.
>  	 */
> -	for (i = 1; i < MDIO_MMD_NUM && devs_in_pkg == 0 &&
> -	     (devs_in_pkg & 0x1fffffff) == 0x1fffffff; i++) {
> +	for (i = 1; i < MDIO_MMD_NUM && (devs_in_pkg == 0 ||
> +	     (devs_in_pkg & 0x1fffffff) == 0x1fffffff); i++) {
>  		if (i == MDIO_MMD_VEND1 || i == MDIO_MMD_VEND2) {
>  			/* Check that there is a device present at this
>  			 * address before reading the devices-in-package
> -- 
> 2.25.1
> 

Thanks,
-Vladimir