netdev - Re: [PATCH net-next 6/6] net: ethernet: fs

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: <20240828163224.GT1368797@kernel.org>
Date: Wed, 28 Aug 2024 17:32:24 +0100
From: Simon Horman <horms@...nel.org>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: davem@...emloft.net, Pantelis Antoniou <pantelis.antoniou@...il.com>,
	Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Russell King <linux@...linux.org.uk>,
	Christophe Leroy <christophe.leroy@...roup.eu>,
	Florian Fainelli <f.fainelli@...il.com>,
	Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
	Herve Codina <herve.codina@...tlin.com>,
	linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> fs_enet is a quite old but still used Ethernet driver found on some NXP
> devices. It has support for 10/100 Mbps ethernet, with half and full
> duplex. Some variants of it can use RMII, while other integrations are
> MII-only.
> 
> Add phylink support, thus removing custom fixed-link hanldling.
> 
> This also allows removing some internal flags such as the use_rmii flag.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.com>

Hi Maxime,

Some minor issues from my side: not a full review by any stretch of
the imagination.

...

> @@ -911,7 +894,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  	if (!IS_ERR(clk)) {
>  		ret = clk_prepare_enable(clk);
>  		if (ret)
> -			goto out_deregister_fixed_link;
> +			goto out_phylink;
>  
>  		fpi->clk_per = clk;
>  	}

This goto will result in a dereference of fep.
But fep is not initialised until the following line,
which appears a little below this hunk.

	fep = netdev_priv(ndev);

This goto will also result in the function returning without
releasing clk.

Both flagged by Smatch.

> @@ -936,6 +919,26 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  	fep->fpi = fpi;
>  	fep->ops = ops;
>  
> +	fep->phylink_config.dev = &ndev->dev;
> +	fep->phylink_config.type = PHYLINK_NETDEV;
> +	fep->phylink_config.mac_capabilities = MAC_10 | MAC_100;
> +
> +	__set_bit(PHY_INTERFACE_MODE_MII,
> +		  fep->phylink_config.supported_interfaces);
> +
> +	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec"))
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  fep->phylink_config.supported_interfaces);
> +
> +	phylink = phylink_create(&fep->phylink_config, dev_fwnode(fep->dev),
> +				 phy_mode, &fs_enet_phylink_mac_ops);
> +	if (IS_ERR(phylink)) {
> +		ret = PTR_ERR(phylink);
> +		goto out_free_fpi;

This also appears to leak clk, as well as ndev.

I didn't look for other cases.

> +	}
> +
> +	fep->phylink = phylink;
> +
>  	ret = fep->ops->setup_data(ndev);
>  	if (ret)
>  		goto out_free_dev;
> @@ -968,8 +971,6 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  
>  	ndev->ethtool_ops = &fs_ethtool_ops;
>  
> -	netif_carrier_off(ndev);
> -
>  	ndev->features |= NETIF_F_SG;
>  
>  	ret = register_netdev(ndev);
> @@ -988,10 +989,8 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  	free_netdev(ndev);
>  out_put:
>  	clk_disable_unprepare(fpi->clk_per);
> -out_deregister_fixed_link:
> -	of_node_put(fpi->phy_node);
> -	if (of_phy_is_fixed_link(ofdev->dev.of_node))
> -		of_phy_deregister_fixed_link(ofdev->dev.of_node);
> +out_phylink:
> +	phylink_destroy(fep->phylink);
>  out_free_fpi:
>  	kfree(fpi);
>  	return ret;

...