netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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()
Date: Thu, 4 Nov 2021 12:35:17 +0000	[thread overview]
Message-ID: <YYPThd7aX+TBWslz@shell.armlinux.org.uk> (raw)
In-Reply-To: <87pmrgjhk4.fsf@waldekranz.com>

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!

  reply	other threads:[~2021-11-04 12:35 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-01 18:28 [RFC PATCH] net: phy/mdio: enable mmd indirect access through phy_mii_ioctl() Grygorii Strashko
2021-11-01 19:33 ` Andrew Lunn
2021-11-01 19:54   ` Russell King (Oracle)
2021-11-02  0:49     ` Andrew Lunn
2021-11-02 12:39       ` Russell King (Oracle)
2021-11-02 17:13         ` Andrew Lunn
2021-11-02 19:46           ` Sean Anderson
2021-11-02 23:38             ` Russell King (Oracle)
2021-11-04 15:05               ` Sean Anderson
2021-11-02 17:19         ` Grygorii Strashko
2021-11-02 17:41           ` Russell King (Oracle)
2021-11-02 18:37             ` Grygorii Strashko
2021-11-02 19:12               ` Grygorii Strashko
2021-11-02 21:46                 ` Andrew Lunn
2021-11-02 22:22                   ` Grygorii Strashko
2021-11-03  0:27                     ` Andrew Lunn
2021-11-03 18:42                       ` Grygorii Strashko
2021-11-03 19:36                         ` Andrew Lunn
2021-11-04 11:17                           ` Tobias Waldekranz
2021-11-04 12:35                             ` Russell King (Oracle) [this message]
2021-11-04 12:40                               ` Russell King (Oracle)
2021-11-04 13:13                                 ` Tobias Waldekranz
2021-11-04 13:06                               ` Tobias Waldekranz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YYPThd7aX+TBWslz@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=grygorii.strashko@ti.com \
    --cc=hkallweit1@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=sean.anderson@seco.com \
    --cc=tobias@waldekranz.com \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).