netdev - Re: [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx

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: <20241003111811.GJ1310185@kernel.org>
Date: Thu, 3 Oct 2024 12:18:11 +0100
From: Simon Horman <horms@...nel.org>
To: Vitalii Mordan <mordan@...ras.ru>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	lvc-project@...uxtesting.org, Fedor Pchelkin <pchelkin@...ras.ru>,
	Alexey Khoroshilov <khoroshilov@...ras.ru>,
	Vadim Mutilin <mutilin@...ras.ru>
Subject: Re: [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx_clk
 handling routines

On Mon, Sep 30, 2024 at 09:37:15PM +0300, Vitalii Mordan wrote:
> If the clock dwmac->tx_clk was not enabled in intel_eth_plat_probe,
> it should not be disabled in any path.
> 
> Conversely, if it was enabled in intel_eth_plat_probe, it must be disabled
> in all error paths to ensure proper cleanup.
> 
> Found by Linux Verification Center (linuxtesting.org) with Klever.
> 
> Fixes: 9efc9b2b04c7 ("net: stmmac: Add dwmac-intel-plat for GBE driver")
> Signed-off-by: Vitalii Mordan <mordan@...ras.ru>
> ---
>  .../ethernet/stmicro/stmmac/dwmac-intel-plat.c   | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> index d68f0c4e7835..2a2893f2f2a8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> @@ -108,7 +108,12 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>  			if (IS_ERR(dwmac->tx_clk))
>  				return PTR_ERR(dwmac->tx_clk);
>  
> -			clk_prepare_enable(dwmac->tx_clk);
> +			ret = clk_prepare_enable(dwmac->tx_clk);
> +			if (ret) {
> +				dev_err(&pdev->dev,
> +					"Failed to enable tx_clk\n");
> +				return ret;
> +			}
>  
>  			/* Check and configure TX clock rate */
>  			rate = clk_get_rate(dwmac->tx_clk);
> @@ -117,6 +122,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>  				rate = dwmac->data->tx_clk_rate;
>  				ret = clk_set_rate(dwmac->tx_clk, rate);
>  				if (ret) {
> +					clk_disable_unprepare(dwmac->tx_clk);
>  					dev_err(&pdev->dev,
>  						"Failed to set tx_clk\n");
>  					return ret;

Hi Vitalii,

I think that unwinding using a goto label would be more idiomatic here
and in the following changes to intel_eth_plat_probe().

> @@ -131,6 +137,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>  			rate = dwmac->data->ptp_ref_clk_rate;
>  			ret = clk_set_rate(plat_dat->clk_ptp_ref, rate);
>  			if (ret) {
> +				if (dwmac->data->tx_clk_en)
> +					clk_disable_unprepare(dwmac->tx_clk);
>  				dev_err(&pdev->dev,
>  					"Failed to set clk_ptp_ref\n");
>  				return ret;
> @@ -150,7 +158,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>  
>  	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
>  	if (ret) {
> -		clk_disable_unprepare(dwmac->tx_clk);
> +		if (dwmac->data->tx_clk_en)
> +			clk_disable_unprepare(dwmac->tx_clk);

Smatch warns that dwmac->data may be NULL here.

>  		return ret;
>  	}
>  
> @@ -162,7 +171,8 @@ static void intel_eth_plat_remove(struct platform_device *pdev)
>  	struct intel_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
>  
>  	stmmac_pltfr_remove(pdev);
> -	clk_disable_unprepare(dwmac->tx_clk);
> +	if (dwmac->data->tx_clk_en)

And I wonder if it can be NULL here too.

> +		clk_disable_unprepare(dwmac->tx_clk);
>  }
>  
>  static struct platform_driver intel_eth_plat_driver = {
> -- 
> 2.25.1
> 
>