netdev - Re: [PATCH net] net: mdio: Prevent Clause 45 scan on SMSC PHYs

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: Tue, 2 Jan 2024 10:14:29 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Michael Walle <michael@...le.cc>
Cc: ezra@...ergy-village.org, Andrew Lunn <andrew@...n.ch>,
 Russell King <linux@...linux.org.uk>, Tristram Ha
 <Tristram.Ha@...rochip.com>, Jesse Brandeburg <jesse.brandeburg@...el.com>,
 netdev@...r.kernel.org
Subject: Re: [PATCH net] net: mdio: Prevent Clause 45 scan on SMSC PHYs

On 02.01.2024 09:50, Michael Walle wrote:
> Hi,
> 
>>> Since commit 1a136ca2e089 ("net: mdio: scan bus based on bus
>>> capabilities for C22 and C45") our AT91SAM9G25-based GARDENA smart
>>> Gateway will no longer boot.
>>>
>>> Prior to the mentioned change, probe_capabilities would be set to
>>> MDIOBUS_NO_CAP (0) and therefore, no Clause 45 scan was performed.
>>> Running a Clause 45 scan on an SMSC/Microchip LAN8720A PHY will (at
>>> least with our setup) considerably slow down kernel startup and
>>> ultimately result in a board reset.
>>>
>>> AFAICT all SMSC/Microchip PHYs are Clause 22 devices. Some have a
>>> "Clause 45 protection" feature (e.g. LAN8830) and others like the
>>> LAN8804 will explicitly state the following in the datasheet:
>>>
>>>     This device may respond to Clause 45 accesses and so must not be
>>>     mixed with Clause 45 devices on the same MDIO bus.
> 
> If implemented correctly, c22 phys should never respond to c45
> accesses. Correct? So the "Clause 45 protection" sounds like the
> normal behavior here and the "may respond to c45 accesses" looks
> like it's broken.
> 
>> I'm not convinced that some heuristic based on vendors is a
>> sustainable approach. Also I'd like to avoid (as far as possible)
>> that core code includes vendor driver headers. Maybe we could use
>> a new PHY driver flag. Approaches I could think of:
>>
>> Approach 1:
>> Add a PHY driver flag to state: PHY is not c45-access-safe
>> Then c45 scanning would be omitted if at least one c22 PHY
>> with this flag was found.
>>
>> Approach 2:
>> Add a PHY driver flag to state: PHY is c45-access-safe
>> Then c45 scanning would only be done if all found c22 devices
>>
>> Not sure which options have been discussed before. Any feedback
>> welcome.
> 
> I had a similar idea and IIRC Andrew said this would be a layering
> violation. But I can't find the thread anymore.
> 
Due to async probing we may have the case that the driver isn't bound
yet. Right. Maybe there are more reasons.

Then, as a compromise, maybe we can replace the OUI checks with a blacklist,
where the entries consist of PHY ID and mask. This would cover the case of
matching all PHY's from a particular vendor, but would also allow to be more
granular.

>> Related: How common are setups where c22 and c45 devices are attached
>> to a single MDIO bus?
> 
> At least we have boards which has c22 and c45 PHYs on one bus. And
> on one board, we even have a Micrel/Microchip PHY on this bus, which
> forces us to use c22-over-c45 for the c45 PHY. I really need to repost my
> c45-over-c22 series, although there was no consensus there unfortunately.
> 
> -michael