netdev - Re: 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] [day] [month] [year] [list]

Message-ID: <9c63817.dd25.18d44aab80c.Coremail.alexious@zju.edu.cn>
Date: Fri, 26 Jan 2024 15:27:35 +0800 (GMT+08:00)
From: alexious@....edu.cn
To: "Simon Horman" <horms@...nel.org>
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: 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>

I think this patch doesn't change a lot since it just refactor the deallocation
ways into unwind ladders while fix a memleak.

> 
> ...
> 
> > @@ -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.

I set these pointers to NULL since its clean up function fjes_hw_cleanup
do so. Personally, I tend to following the existing code style in the 
same module.

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

Well, yes, it's a little bit wired that fjes_hw_init doesn't handle 
errors of fjes_hw_setup and vzalloc of hw->hw_info.trace as normal
functions do.
Maybe another patch need to be submitted to deal with this problem.

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