netdev - Re: [net-next 2/2] net: phy: Fallback to C22 access if needed in phy_mii

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: <365313cb-c767-414a-8b4b-97882854e9b6@lunn.ch>
Date: Fri, 6 Sep 2024 23:04:50 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next 2/2] net: phy: Fallback to C22 access if needed in
 phy_mii_ioctl()

On Fri, Sep 06, 2024 at 11:39:55AM +0200, Niklas Söderlund wrote:
> If a C45 only PHY is attached to a driver that only knows how to talk
> C22 phylib will fallback and use indirect access. This frees the driver
> from having to implement this themself.
> 
> The IOCTL implementation for SIOCGMIIREG and SIOCSMIIREG do not use
> these convenience functions and instead fail if a C45 PHY is used
> together with a driver that only knows how to speak C22.
> 
> Fix this by using the two convince functions that knows when to fallback
> to indirect access to read/write to the MDIO bus when needed.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> ---
>  drivers/net/phy/phy.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 4f3e742907cb..89f52bb123aa 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -342,9 +342,12 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>  		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
>  			prtad = mdio_phy_id_prtad(mii_data->phy_id);
>  			devad = mdio_phy_id_devad(mii_data->phy_id);
> -			ret = mdiobus_c45_read(phydev->mdio.bus, prtad, devad,
> -					       mii_data->reg_num);
>  
> +			mutex_lock(&phydev->mdio.bus->mdio_lock);
> +			ret = mmd_phy_read(phydev->mdio.bus, prtad,
> +					   phydev->is_c45, devad,

Using phydev->is_c45 is probably wrong.

mii_data->phy_id is the device on the bus you want to access. It does
not need to be the same device as the MAC is using. Just because the
device the MAC is using is a c45 device does not mean the device you
are trying to access is.

Maybe i gave you some bad advice. Sorry.

This API is reasonably well known to be a foot gun. You should only be
using it for debug, and actually using it, even only to read
registers, can mess up a PHY/phylib.

The API gives you the ability to perform a C22 bus transaction, or a
C45 bus transaction on any arbitrary device. That is all you need for
debug, you can do C45 over C22 in user space. Yes, there are race
conditions, but this API already has race conditions, which is part of
why it is a foot gun.

    Andrew