linux-kernel - Re: [PATCH net-next 7/8] net: phy: Add Synopsys DesignWare XPCS MDIO module

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:   Fri, 5 Jun 2020 18:10:34 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     netdev@...r.kernel.org, Joao Pinto <Joao.Pinto@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 7/8] net: phy: Add Synopsys DesignWare XPCS MDIO
 module

Hi Jose,

I just tripped over a bug while grepping for something else and
reading a bit of this driver:

On Mon, Mar 09, 2020 at 09:36:26AM +0100, Jose Abreu wrote:
> +static int xpcs_read_lpa(struct mdio_xpcs_args *xpcs,
> +			 struct phylink_link_state *state)
> +{
> +	int ret;
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_AN, MDIO_STAT1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!(ret & MDIO_AN_STAT1_LPABLE)) {
> +		phylink_clear(state->lp_advertising, Autoneg);
> +		return 0;
> +	}
> +
> +	phylink_set(state->lp_advertising, Autoneg);
> +
> +	/* Clause 73 outcome */
> +	ret = xpcs_read(xpcs, MDIO_MMD_AN, DW_SR_AN_LP_ABL3);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & DW_C73_2500KX)
> +		phylink_set(state->lp_advertising, 2500baseX_Full);
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_AN, DW_SR_AN_LP_ABL2);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & DW_C73_1000KX)
> +		phylink_set(state->lp_advertising, 1000baseKX_Full);
> +	if (ret & DW_C73_10000KX4)
> +		phylink_set(state->lp_advertising, 10000baseKX4_Full);
> +	if (ret & DW_C73_10000KR)
> +		phylink_set(state->lp_advertising, 10000baseKR_Full);
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_AN, DW_SR_AN_LP_ABL1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & DW_C73_PAUSE)
> +		phylink_set(state->lp_advertising, Pause);
> +	if (ret & DW_C73_ASYM_PAUSE)
> +		phylink_set(state->lp_advertising, Asym_Pause);
> +
> +	linkmode_and(state->lp_advertising, state->lp_advertising,
> +		     state->advertising);

This is incorrect - you should not mask the link partner's advertisement
with our advertisement like this; consider the table in 802.3 for
resolving the pause modes, where simply doing a bitwise-and operation
between the two advertisements would severely restrict the resulting
resolution to either symmetric pause or nothing at all.

You want to do this when you resolve the speed, but only _temporarily_
in order to resolve the speed - you do not want to write back the
result to state->lp_advertising.

You may wish to fix that.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up