netdev - Re: [PATCH net-next] net: phy: avoid NPE if read_page/write

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: <756c3ea2-dd22-7963-bf02-264d6cebc22b@gmail.com>
Date:   Wed, 16 Oct 2019 22:26:57 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        David Miller <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: phy: avoid NPE if read_page/write_page
 callbacks are not available

On 16.10.2019 22:20, Florian Fainelli wrote:
> On 10/16/19 12:53 PM, Heiner Kallweit wrote:
>> Currently there's a bug in the module subsystem [0] preventing load of
>> the PHY driver module on certain systems (as one symptom).
>> This results in a NPE on such systems for the following reason:
>> Instead of the correct PHY driver the genphy driver is loaded that
>> doesn't implement the read_page/write_page callbacks. Every call to
>> phy_read_paged() et al will result in a NPE therefore.
>>
>> In parallel to fixing the root cause we should make sure that this one
>> and maybe similar issues in other subsystems don't result in a NPE
>> in phylib. So let's check for the callbacks before using them and warn
>> once if they are not available.
> 
> Everywhere else in the PHY library we tend to do:
> 
> if (!phydev->drv)
> 	return -EIO;
> 
A driver is bound, but genphy instead of the dedicated one.
I think this check won't catch the error.

> maybe not the best choice for an error code, but we should be consistent.
> 
> Is the issue really that we do have a driver we are bound to, but
> somehow we cannot resolve the read_page/write_page callbacks to a valid
> function pointer?
> 
The issue is that loading the dedicated PHY driver module fails, therefore
the (built-in) genphy driver is bound. Code in the r8169 driver expects
that the dedicated driver for the internal PHY is bound and uses
phy_read_page() et al what fails miserably with the genphy driver.

>>
>> [0] https://marc.info/?t=157072642100001&r=1&w=2
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/phy/phy-core.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> index 9412669b5..0ae1722ba 100644
>> --- a/drivers/net/phy/phy-core.c
>> +++ b/drivers/net/phy/phy-core.c
>> @@ -689,11 +689,17 @@ EXPORT_SYMBOL_GPL(phy_modify_mmd);
>>  
>>  static int __phy_read_page(struct phy_device *phydev)
>>  {
>> +	if (WARN_ONCE(!phydev->drv->read_page, "read_page callback not available, PHY driver not loaded?\n"))
>> +		return -EOPNOTSUPP;
>> +
>>  	return phydev->drv->read_page(phydev);
>>  }
>>  
>>  static int __phy_write_page(struct phy_device *phydev, int page)
>>  {
>> +	if (WARN_ONCE(!phydev->drv->write_page, "write_page callback not available, PHY driver not loaded?\n"))
>> +		return -EOPNOTSUPP;
>> +
>>  	return phydev->drv->write_page(phydev, page);
>>  }
>>  
>>
> 
>