Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 4 Nov 2021 12:35:17 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Tobias Waldekranz <tobias@waldekranz.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Grygorii Strashko <grygorii.strashko@ti.com>,
        "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        linux-kernel@vger.kernel.org,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Sean Anderson <sean.anderson@seco.com>
Subject: Re: [RFC PATCH] net: phy/mdio: enable mmd indirect access through
 phy_mii_ioctl()
Message-ID: <YYPThd7aX+TBWslz@shell.armlinux.org.uk>
References: <bc9df441-49bf-5c8a-891c-cc3f0db00aba@ti.com>
 <YYF4ZQHqc1jJsE/+@shell.armlinux.org.uk>
 <e18f17bd-9e77-d3ef-cc1e-30adccb7cdd5@ti.com>
 <828e2d69-be15-fe69-48d8-9cfc29c4e76e@ti.com>
 <YYGxvomL/0tiPzvV@lunn.ch>
 <8d24c421-064c-9fee-577a-cbbf089cdf33@ti.com>
 <YYHXcyCOPiUkk8Tz@lunn.ch>
 <01a0ebf9-5d3f-e886-4072-acb9bf418b12@ti.com>
 <YYLk0dEKX2Jlq0Se@lunn.ch>
 <87pmrgjhk4.fsf@waldekranz.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87pmrgjhk4.fsf@waldekranz.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Nov 04, 2021 at 12:17:47PM +0100, Tobias Waldekranz wrote:
> On Wed, Nov 03, 2021 at 20:36, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Wed, Nov 03, 2021 at 08:42:07PM +0200, Grygorii Strashko wrote:
> >> 
> >> 
> >> On 03/11/2021 02:27, Andrew Lunn wrote:
> >> > > > What i find interesting is that you and the other resent requester are
> >> > > > using the same user space tool. If you implement C45 over C22 in that
> >> > > > tool, you get your solution, and it will work for older kernels as
> >> > > > well. Also, given the diverse implementations of this IOTCL, it
> >> > > > probably works for more drivers than just those using phy_mii_ioctl().
> >> > > 
> >> > > Do you mean change uapi, like
> >> > >   add mdio_phy_id_is_c45_over_c22() and
> >> > >   flag #define MDIO_PHY_ID_C45_OVER_C22 0x4000?
> >> > 
> >> > No, i mean user space implements C45 over C22. Make phytool write
> >> > MII_MMD_CTRL and MII_MMD_DATA to perform a C45 over C22.
> >> 
> >> Now I give up - as mentioned there is now way to sync User space vs Kernel
> >> MMD transactions and so no way to get trusted results.
> 
> Except that there is a way: https://github.com/wkz/mdio-tools

I'm guessing that this hasn't had much in the way of review, as it has
a nice exploitable bug - you really want "pc" to be unsigned in
mdio_nl_eval(), otherwise one can write a branch instruction that makes
"pc" negative.

Also it looks like one can easily exploit this to trigger any of your
BUG_ON()/BUG() statements, thereby crashing while holding the MDIO bus
lock causing a denial of service attack.

I also see nothing that protects against any user on a system being
able to use this interface, so the exploits above can be triggered by
any user. Moreover, this lack of protection means any user on the
system can use this interface to write to a PHY.

Given that some PHYs today contain firmware, this gives anyone access
to reprogram the PHY firmware, possibly introducing malicious firmware.

I hope no one is using this module in a production environment.

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