netdev - Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over

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 PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 31 Mar 2022 12:31:49 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Michael Walle <michael@...le.cc>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S . Miller" <davem@...emloft.net>,
        Xu Liang <lxu@...linear.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

On Thu, Mar 24, 2022 at 01:41:44AM +0100, Andrew Lunn wrote:
ydev->c45_over_c22 we are currently in a bad shape for. We cannot
> reliably say the bus master supports C45. If the bus capabilities say
> C22 only, we can set phydev->c45_over_c22. If the bus capabilities
> list C45, we can set it false. But that only covers a few busses, most
> don't have any capabilities set. We can try a C45 access and see if we
> get an -EOPNOTSUPP, in which case we can set phydev->c45_over_c22. But
> the bus driver could also do the wrong thing, issue a C22 transfer and
> give us back rubbish.

Unfortunately, trying a C45 access will be very hit and miss - we
need to fix all the MDIO drivers before we do that to check the
access type. Many don't, and worse, many assume a C22 formatted
access request, and just try throwing the PHY address and register
address into the register fields without any masking. The result is
that a C45 access will set random bits in the register.

For example:
drivers/net/mdio/mdio-bcm-iproc.c (no bus capability):

        /* Prepare the read operation */
        cmd = (MII_DATA_TA_VAL << MII_DATA_TA_SHIFT) |
                (reg << MII_DATA_RA_SHIFT) |
                (phy_id << MII_DATA_PA_SHIFT) |
                BIT(MII_DATA_SB_SHIFT) |
                (MII_DATA_OP_READ << MII_DATA_OP_SHIFT);

        writel(cmd, priv->base + MII_DATA_OFFSET);

Similar is true for:
drivers/net/mdio/mdio-bcm-unimac.c (no bus capability)
drivers/net/mdio/mdio-hisi-femac.c (no bus capability)
drivers/net/mdio/mdio-moxart.c (no bus capability)
drivers/net/mdio/mdio-mscc-miim.c (no bus capability)
drivers/net/mdio/mdio-mux-bcm6368.c (no bus capability)
drivers/net/mdio/mdio-mux-bcm-iproc.c (no bus capability)
drivers/net/mdio/mdio-sun4i.c (no bus capability)

These truncate the fields, and fwics they don't set the bus type:
drivers/net/mdio/mdio-xgene.c (for the "rgmii" only bus and no bus capability)

So all of the above need at the very least code added to reject a C45
"dev" or "phy_id" address, or they need to set the bus capability
correctly.

My feeling is that the introduction of the bus capability hasn't done
much to improve this situation; it was introduced to hint at whether a
bus is safe for clause 45 accesses, but it hasn't actually solved this
problem because we patently still have this same issue today.

I think we just need to bite the bullet and audit all the MDIO drivers
we currently have, checking what the results would be if they were
passed a C45 access request, and make them reject such a request if
the register address or phy address obviously overflows into different
register fields and also mark them as C22-only.

I can't see any other reasonable option.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!