netdev - Re: [PATCH v3] 3c59x: Fix memory leaks in vortex

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]

Date:	Tue, 23 Dec 2014 10:43:13 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	Jia-Ju Bai <baijiaju1990@....com>
Cc:	davem@...emloft.net, ebiederm@...ssion.com,
	dingtianhong@...wei.com, paul.gortmaker@...driver.com,
	justinvanwijngaarden@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH v3] 3c59x: Fix memory leaks in vortex_open

On Tue, Dec 23, 2014 at 11:00:01PM +0800, Jia-Ju Bai wrote:
> Thanks for the reply!
> 
> >This doesn't make sense.  We free all the skbs in vortex_open if we don't
> >allocate all of them (the if (i != RX_RING_SIZE) check), the only place we miss
> >is if vortex_up fails, and you didn't remove the if (!retval) goto out check, so
> >this code won't get run appropriately.
> 
> In the code, when vortex_up is failed and does not returns 0,
> "if (!retval)" is failed and "goto out" is not executed, so error handling
> code
> below is executed, including my added code.
> Is it right?
> 
Yup, you're right, I missed the sense of the check.

> >
> >That said, it does seem we need to clean up if vortex_up fails, but it would
> >seem to me to be easier to just call vortex_close if it does, since that will do
> >all of the approriate cleanup.
> >
> >Neil
> >
> 
> In the code, vortex_close does too many releasing operations, such as free
> vp->tx_skbuff, but vortex_open only allocates memory for vp->rx_skbuff
> before
> vortex_up is called.
> So I think it is enough to just free the memory of vp->rx_skbuff when
> vortex_up
> is failed.
> 
No, I don't think so.  vortex_close predicates each free with a NULL check, so
if its not been allocated, it shouldn't be freed.  vortex_close also puts the
adapter back into a known state (undoing all the setup that vortex_open does).
I really think its better to go with the proper close path than just unwinding
the allocation

Neil

> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html