linux-kernel - Re: [RFC PATCH net-next 5/8] net: phy: realtek: use phy_read

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]

Date:   Sun, 23 Apr 2023 19:01:55 +0100
From:   Daniel Golle <daniel@...rotopia.org>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        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>,
        Chen Minqiang <ptpt52@...il.com>,
        Chukun Pan <amadeus@....edu.cn>,
        Yevhen Kolomeiko <jarvis2709@...il.com>,
        Alexander Couzens <lynxis@...0.eu>
Subject: Re: [RFC PATCH net-next 5/8] net: phy: realtek: use phy_read_paged
 instead of open coding

On Sat, Apr 22, 2023 at 05:11:57PM +0200, Heiner Kallweit wrote:
> On 22.04.2023 13:48, Daniel Golle wrote:
> > Instead of open coding a paged read, use the phy_read_paged function
> > in rtlgen_supports_2_5gbps.
> > 
> > Signed-off-by: Daniel Golle <daniel@...rotopia.org>
> > ---
> >  drivers/net/phy/realtek.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index f97b5e49fae58..62fb965b6d338 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -735,9 +735,7 @@ static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
> >  {
> >  	int val;
> >  
> > -	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa61);
> > -	val = phy_read(phydev, 0x13);
> > -	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
> > +	val = phy_read_paged(phydev, 0xa61, 0x13);
> >  
> >  	return val >= 0 && val & RTL_SUPPORTS_2500FULL;
> >  }
> 
> I remember I had a reason to open-code it, it took me some minutes
> to recall it.
> phy_read_paged() calls __phy_read_page() that relies on phydev->drv
> being set. phydev->drv is set in phy_probe(). And probing is done
> after matching. __phy_read_paged() should have given you a warning.
> Did you test this patch? If yes and you didn't get the warning,
> then apparently I miss something.
>

Yes, you are right, this change was a bit too naive and causes a
NULL pointer dereference e.g. for the r8169 driver which also uses
the RealTek Ethernet PHY driver.
My main concern and original motivation was the lack of mutex protection
for the paged read operation. I suggest to rather make this change
instead:

>From 4dd2cc9b91ecb25f278a2c55e07e6455e9000e6b Mon Sep 17 00:00:00 2001
From: Daniel Golle <daniel@...rotopia.org>
Date: Sun, 23 Apr 2023 18:47:45 +0100
Subject: [PATCH] net: phy: realtek: make sure paged read is protected by mutex

As we cannot rely on phy_read_paged function before the PHY is
identified, the paged read in rtlgen_supports_2_5gbps needs to be open
coded as it is being called by the match_phy_device function, ie. before
.read_page and .write_page have been populated.

Make sure it is also protected by the MDIO bus mutex and use
rtl821x_write_page instead of 3 individually locked MDIO bus operations.

Signed-off-by: Daniel Golle <daniel@...rotopia.org>
---
 drivers/net/phy/realtek.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index f97b5e49fae5..c27ec4e99fc2 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -735,9 +735,11 @@ static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
 {
 	int val;
 
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa61);
-	val = phy_read(phydev, 0x13);
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
+	mutex_lock(&phydev->mdio.bus->mdio_lock);
+	rtl821x_write_page(phydev, 0xa61);
+	val = __phy_read(phydev, 0x13);
+	rtl821x_write_page(phydev, 0);
+	mutex_unlock(&phydev->mdio.bus->mdio_lock);
 
 	return val >= 0 && val & RTL_SUPPORTS_2500FULL;
 }
-- 
2.40.0