netdev - Re: [PATCH RFC 2/4] net: phy: allow to bind genphy driver at probe time

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: <7225e653-6f93-63fc-8d61-a712318d1949@gmail.com>
Date:   Wed, 14 Aug 2019 01:02:10 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>, Marek Behun <marek.behun@....cz>,
        David Miller <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC 2/4] net: phy: allow to bind genphy driver at probe
 time

On 14.08.2019 00:53, Florian Fainelli wrote:
> On 8/13/19 2:25 PM, Heiner Kallweit wrote:
>> In cases like a fixed phy that is never attached to a net_device we
>> may want to bind the genphy driver at probe time. Setting a PHY ID of
>> 0xffffffff to bind the genphy driver would fail due to a check in
>> get_phy_device(). Therefore let's change the PHY ID the genphy driver
>> binds to to 0xfffffffe. This still shouldn't match any real PHY,
>> and it will pass the check in get_phy_devcie().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/phy/phy_device.c | 3 +--
>>  include/linux/phy.h          | 4 ++++
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 163295dbc..54f80af31 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -2388,8 +2388,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n)
>>  EXPORT_SYMBOL(phy_drivers_unregister);
>>  
>>  static struct phy_driver genphy_driver = {
>> -	.phy_id		= 0xffffffff,
>> -	.phy_id_mask	= 0xffffffff,
>> +	PHY_ID_MATCH_EXACT(GENPHY_ID),
>>  	.name		= "Generic PHY",
>>  	.soft_reset	= genphy_no_soft_reset,
>>  	.get_features	= genphy_read_abilities,
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 5ac7d2137..3b07bce78 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -37,6 +37,10 @@
>>  #define PHY_1000BT_FEATURES	(SUPPORTED_1000baseT_Half | \
>>  				 SUPPORTED_1000baseT_Full)
>>  
>> +#define GENPHY_ID_HIGH		0xffffU
>> +#define GENPHY_ID_LOW		0xfffeU
>> +#define GENPHY_ID		((GENPHY_ID_HIGH << 16) | GENPHY_ID_LOW)
> 
> This is a possible user ABI change here, if there is anything that
> relies on reading 0xffff_ffff as a valid PHY OUI, you would be breaking
> it. We might as well try to assign ourselves a specific PHY OUI, very
> much like the Linux USB hubs show up with a Linux Foundation vendor ID.
> 

I see the point. However in get_phy_device() we have the following check
that should cause a PHY with ID 0xffff_ffff to be ignored. Therefore
I doubt there's any such PHY ID in use.

	/* If the phy_id is mostly Fs, there is no device there */
	if ((phy_id & 0x1fffffff) == 0x1fffffff)
		return ERR_PTR(-ENODEV);

Heiner