netdev - Re: [PATCH] fjes: fix memleaks in fjes_hw

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: <20240122210538.GJ126470@kernel.org>
Date: Mon, 22 Jan 2024 21:05:38 +0000
From: Simon Horman <horms@...nel.org>
To: Zhipeng Lu <alexious@....edu.cn>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Taku Izumi <izumi.taku@...fujitsu.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fjes: fix memleaks in fjes_hw_setup

On Tue, Jan 23, 2024 at 01:24:42AM +0800, Zhipeng Lu wrote:
> In fjes_hw_setup, it allocates several memory and delay the deallocation
> to the fjes_hw_exit in fjes_probe through the following call chain:
> 
> fjes_probe
>   |-> fjes_hw_init
>         |-> fjes_hw_setup
>   |-> fjes_hw_exit
> 
> However, when fjes_hw_setup fails, fjes_hw_exit won't be called and thus
> all the resources allocated in fjes_hw_setup will be leaked. In this
> patch, we free those resources in fjes_hw_setup and prevents such leaks.
> 
> Fixes: 2fcbca687702 ("fjes: platform_driver's .probe and .remove routine")
> Signed-off-by: Zhipeng Lu <alexious@....edu.cn>

Hi Zhipeng Lu,

It looks like the last non-trivial change to this driver was in 2016.
So perhaps it is better to leave it be.

But if not, this patch does look correct to me.

Reviewed-by: Simon Horman <horms@...nel.org>

...

> @@ -273,6 +277,25 @@ static int fjes_hw_setup(struct fjes_hw *hw)
>  	fjes_hw_init_command_registers(hw, &param);
>  
>  	return 0;
> +
> +free_epbuf:
> +	for (epidx = 0; epidx < hw->max_epid ; epidx++) {
> +		if (epidx == hw->my_epid)
> +			continue;
> +		fjes_hw_free_epbuf(&hw->ep_shm_info[epidx].tx);
> +		fjes_hw_free_epbuf(&hw->ep_shm_info[epidx].rx);
> +	}
> +	fjes_hw_free_shared_status_region(hw);
> +free_res_buf:
> +	kfree(hw->hw_info.res_buf);
> +	hw->hw_info.res_buf = NULL;
> +free_req_buf:
> +	kfree(hw->hw_info.req_buf);
> +	hw->hw_info.req_buf = NULL;
> +free_ep_info:
> +	kfree(hw->ep_shm_info);
> +	hw->ep_shm_info = NULL;
> +	return result;

FWIIW, I'm not sure it is necessary to set these pointers NULL,
although it doesn't do any harm.

Also, if this function returns an error,
does the caller (fjes_hw_init()) leak hw->hw_info.trace?

>  }
>  
>  static void fjes_hw_cleanup(struct fjes_hw *hw)
> -- 
> 2.34.1
>