netdev - Re: [PATCH] net: stmmac: call stmmac_remove_config_dt() in error paths in stmmac_probe_config

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <21d372a2-479a-4ea5-a60a-3693b8f0e8ba@kernel.org>
Date: Tue, 17 Dec 2024 11:58:40 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Joe Hattori <joe@...is.s.u-tokyo.ac.jp>, alexandre.torgue@...s.st.com,
 joabreu@...opsys.com, andrew+netdev@...n.ch, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 mcoquelin.stm32@...il.com
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH] net: stmmac: call stmmac_remove_config_dt() in error
 paths in stmmac_probe_config_dt()

On 17/12/2024 04:32, Joe Hattori wrote:
> Current implementation of stmmac_probe_config_dt() does not release the
> OF node reference obtained by of_parse_phandle() when stmmac_dt_phy()
> fails, thus call stmmac_remove_config_dt(). The error_hw_init and
> error_pclk_get labels can be removed as just calling
> stmmac_remove_config_dt() suffices. Also, remove the first argument of
> stmmac_remove_config_dt() as it is not used.
> 
> This bug was found by an experimental static analysis tool that I am
> developing.
> 
> Fixes: 4838a5405028 ("net: stmmac: Fix wrapper drivers not detecting PHY")
> Signed-off-by: Joe Hattori <joe@...is.s.u-tokyo.ac.jp>
> ---
>  .../ethernet/stmicro/stmmac/stmmac_platform.c | 37 +++++++------------
>  1 file changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 3ac32444e492..4f1a9f7aae6e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -407,13 +407,11 @@ static int stmmac_of_get_mac_mode(struct device_node *np)
>  
>  /**
>   * stmmac_remove_config_dt - undo the effects of stmmac_probe_config_dt()
> - * @pdev: platform_device structure
>   * @plat: driver data platform structure
>   *
>   * Release resources claimed by stmmac_probe_config_dt().
>   */
> -static void stmmac_remove_config_dt(struct platform_device *pdev,
> -				    struct plat_stmmacenet_data *plat)
> +static void stmmac_remove_config_dt(struct plat_stmmacenet_data *plat)
>  {
>  	clk_disable_unprepare(plat->stmmac_clk);
>  	clk_disable_unprepare(plat->pclk);
> @@ -436,7 +434,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  	struct plat_stmmacenet_data *plat;
>  	struct stmmac_dma_cfg *dma_cfg;
>  	int phy_mode;
> -	void *ret;
>  	int rc;
>  
>  	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
> @@ -490,8 +487,10 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  		dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n");
>  
>  	rc = stmmac_mdio_setup(plat, np, &pdev->dev);
> -	if (rc)
> +	if (rc) {
> +		stmmac_remove_config_dt(plat);
>  		return ERR_PTR(rc);
You now double unprepare the clocks. This is either buggy or just
confusing (assuming clocks are NULL).

Best regards,
Krzysztof