netdev - Re: [PATCH RFC] net: mdio_bus: make check in mdiobus_prevent_c45

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] [day] [month] [year] [list]

Message-ID: <8dbad648-561d-407a-9d2f-41175acccff4@gmail.com>
Date: Tue, 2 Jan 2024 16:47:39 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC] net: mdio_bus: make check in mdiobus_prevent_c45_scan
 more granular

On 02.01.2024 16:18, Russell King (Oracle) wrote:
> On Tue, Jan 02, 2024 at 03:38:05PM +0100, Heiner Kallweit wrote:
>> Matching on OUI level is a quite big hammer. So let's make matching
>> more granular.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>> This is what I'm thinking of. Maybe the problem of misbehaving
>> on c45 access affects certain groups of PHY's only.
>> Then we don't have to blacklist all PHY's from this vendor.
>> What do you think?
>> ---
>>  drivers/net/phy/mdio_bus.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index 6cf73c156..848d5d2d6 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>> @@ -621,19 +621,27 @@ static int mdiobus_scan_bus_c45(struct mii_bus *bus)
>>   */
>>  static bool mdiobus_prevent_c45_scan(struct mii_bus *bus)
>>  {
>> -	int i;
>> +	const struct {
>> +		u32 phy_id;
>> +		u32 phy_id_mask;
>> +	} id_list[] = {
>> +		{ MICREL_OUI << 10, GENMASK(31, 10) },
>> +	};
> 
> Do we need a new structure? Would struct mdio_device_id do (which
> actually has exactly the same members with exactly the same names in
> exactly the same order.)
> 
> Also, as this is not static, the compiler will need to generate code
> to initialise the structure, possibly storing a copy of it in the
> .data segment and memcpy()ing it onto the kernel stack. I suggest
> marking it static to avoid that unnecessary hidden code complexity.
> 
Both good points. I missed the static declaration.

>> +		for (j = 0; j < ARRAY_SIZE(id_list); j++) {
>> +			u32 mask = id_list[j].phy_id_mask;
>> +
>> +			if ((phydev->phy_id & mask) == (id_list[j].phy_id & mask))
> 
> 			if (phy_id_compare(phydev->phy_id, id_list[j].phy_id,
> 					   id_list[j].phy_id_mask))
> 
> Or it could be:
> 
> 			const struct mdio_device_id *id = id_list + j;
> 
> 			if (phy_id_compare(phydev->phy_id, id->phy_id,
> 					   id->phy_id_mask))
> 
This looks best to me.