Message-ID: <cd0d9c40-d07b-e2ab-b068-d0bcb4685d09@bang-olufsen.dk>
Date:   Tue, 17 Aug 2021 21:25:28 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Vladimir Oltean <vladimir.oltean@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
CC:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Frank Rowand <frowand.list@...il.com>,
        Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH net] net: dsa: sja1105: fix use-after-free after calling
 of_find_compatible_node, or worse

Hi Vladimir,

On 8/17/21 4:52 PM, Vladimir Oltean wrote:
> It seems that of_find_compatible_node has a weird calling convention in
> which it calls of_node_put() on the "from" node argument, instead of
> leaving that up to the caller. This comes from the fact that
> of_find_compatible_node with a non-NULL "from" argument it only supposed
> to be used as the iterator function of for_each_compatible_node(). OF
> iterator functions call of_node_get on the next OF node and of_node_put()
> on the previous one.
> 
> When of_find_compatible_node calls of_node_put, it actually never
> expects the refcount to drop to zero, because the call is done under the
> atomic devtree_lock context, and when the refcount drops to zero it
> triggers a kobject and a sysfs file deletion, which assume blocking
> context.
> 
> So any driver call to of_find_compatible_node is probably buggy because
> an unexpected of_node_put() takes place.
> 
> What should be done is to use the of_get_compatible_child() function.

I have an observation that's slightly out of the scope of your patch, 
but I'll post here on the off chance that you find it relevant. 
Apologies if it's out of place.

Do these integrated NXP PHYs use a specific PHY driver, or do they just 
use the Generic PHY driver? If the former is the case, do you experience 
that the PHY driver fails to get probed during mdiobus registration if 
the kernel uses fw_devlink=on?

In my case I am writing a new subdriver for realtek-smi, a DSA driver 
which registers an internal MDIO bus analogously to sja1105, which is 
why I'm asking. I noticed a deferred probe of the PHY driver because the 
supplier (ethernet-switch) is not ready - presumably because all of this 
is happening in the probe of the switch driver. See below:

[   83.653213] device_add:3270: device: 'SMI-0': device_add
[   83.653905] device_pm_add:136: PM: Adding info for No Bus:SMI-0
[   83.654055] device_add:3270: device: 
'platform:ethernet-switch--mdio_bus:SMI-0': device_add
[   83.654224] device_link_add:843: mdio_bus SMI-0: Linked as a sync 
state only consumer to ethernet-switch
[   83.654291] libphy: SMI slave MII: probed
...
[   83.659809] device_add:3270: device: 'SMI-0:00': device_add
[   83.659883] bus_add_device:447: bus: 'mdio_bus': add device SMI-0:00
[   83.659970] device_pm_add:136: PM: Adding info for mdio_bus:SMI-0:00
[   83.660122] device_add:3270: device: 
'platform:ethernet-switch--mdio_bus:SMI-0:00': device_add
[   83.660274] devices_kset_move_last:2701: devices_kset: Moving 
SMI-0:00 to end of list
[   83.660282] device_pm_move_last:203: PM: Moving mdio_bus:SMI-0:00 to 
end of list
[   83.660293] device_link_add:859: mdio_bus SMI-0:00: Linked as a 
consumer to ethernet-switch
[   83.660350] __driver_probe_device:736: bus: 'mdio_bus': 
__driver_probe_device: matched device SMI-0:00 with driver RTL8365MB-VC 
Gigabit Ethernet
[   83.660365] device_links_check_suppliers:1001: mdio_bus SMI-0:00: 
probe deferral - supplier ethernet-switch not ready
[   83.660376] driver_deferred_probe_add:138: mdio_bus SMI-0:00: Added 
to deferred list

It's not necessarily fatal because phy_attach_direct will just use the 
Generic PHY driver as a fallback, but it's obviously not the intended 
behaviour.

Perhaps this affects your driver too? Due to lack of hardware I am not 
in a position to test, but a static code analysis suggests it may be if 
you are expecting anything but Generic PHY.

Kind regards,
Alvin

> 
> Fixes: 5a8f09748ee7 ("net: dsa: sja1105: register the MDIO buses for 100base-T1 and 100base-TX")
> Link: https://lore.kernel.org/netdev/20210814010139.kzryimmp4rizlznt@skbuf/
> Suggested-by: Frank Rowand <frowand.list@...il.com>
> Suggested-by: Rob Herring <robh+dt@...nel.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>   drivers/net/dsa/sja1105/sja1105_mdio.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
> index 19aea8fb76f6..705d3900e43a 100644
> --- a/drivers/net/dsa/sja1105/sja1105_mdio.c
> +++ b/drivers/net/dsa/sja1105/sja1105_mdio.c
> @@ -284,8 +284,7 @@ static int sja1105_mdiobus_base_tx_register(struct sja1105_private *priv,
>   	struct mii_bus *bus;
>   	int rc = 0;
>   
> -	np = of_find_compatible_node(mdio_node, NULL,
> -				     "nxp,sja1110-base-tx-mdio");
> +	np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-tx-mdio");
>   	if (!np)
>   		return 0;
>   
> @@ -339,8 +338,7 @@ static int sja1105_mdiobus_base_t1_register(struct sja1105_private *priv,
>   	struct mii_bus *bus;
>   	int rc = 0;
>   
> -	np = of_find_compatible_node(mdio_node, NULL,
> -				     "nxp,sja1110-base-t1-mdio");
> +	np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-t1-mdio");
>   	if (!np)
>   		return 0;
>   
> 

Powered by blists - more mailing lists