netdev - Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps

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: <20181125164513.GD18663@lunn.ch>
Date:   Sun, 25 Nov 2018 17:45:13 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps

On Sun, Nov 25, 2018 at 03:23:42PM +0100, Heiner Kallweit wrote:
> I wondered why ethtool suddenly reports that link partner doesn't
> support aneg and GBit modes. It turned out that this is caused by two
> bugs in conversion to linkmode bitmaps.
> 
> 1. In genphy_read_status the value of phydev->lp_advertising is
>    overwritten, thus GBit modes aren't reported any longer.
> 2. In mii_lpa_to_linkmode_lpa_t the aneg bit was overwritten by the
>    call to mii_adv_to_linkmode_adv_t.

Hi Heiner

Thanks for looking into this.

There are more bugs :-(

static inline void mii_lpa_to_linkmode_lpa_t(unsigned long *lp_advertising,
                                             u32 lpa)
{
        if (lpa & LPA_LPACK)
                linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
                                 lp_advertising);

        mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
}

But

static inline void mii_adv_to_linkmode_adv_t(unsigned long *advertising,
                                             u32 adv)
{
        linkmode_zero(advertising);

        if (adv & ADVERTISE_10HALF)
                linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
                                 advertising);
 
So the Autoneg_BIT gets cleared.

I think the better fix is to take the linkmode_zero() out from here.

Then:

        if (adv & ADVERTISE_10HALF)
               linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
                                advertising);
+	else
+              linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+                                 advertising);

for all the bits mii_adv_to_linkmode_adv_t() looks at.

So mii_adv_to_linkmode_adv_t() only modifies bits it is responsible
for, and leaves the others alone.

    Andrew