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

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

Message-ID: <20141224032728.GA20392@localhost.localdomain>
Date:	Tue, 23 Dec 2014 22:27:28 -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 Wed, Dec 24, 2014 at 10:12:58AM +0800, Jia-Ju Bai wrote:
> On 12/23/2014 11:43 PM, Neil Horman wrote:
> >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
> >
> 
> Firstly, I run my match on the real hardware(3com 3c905B 100Base
> PCI Ethernet Controller) and make vortex_up failed on purpose
> (make "pci_enable_device" in vortex_up failed). During runtime, the driver
> works well and memory leaks are fixed.
> 
> Secondly, I revise the code according to your opinion:
> 
>         retval = vortex_up(dev);
>         if (!retval)
>             goto out;
> 
> +      vortex_close(dev);
> +      return -ENOMEM;
> 
> Then I repeat my experiment, but system hang occurs!
> 
> After adding some "printk"s into the code and running the driver, I find
> the problem's source:
> vortex_close calls vortex_down in runtime, and vortex_down calls
> "del_timer_sync(&vp->rx_oom_timer);" in the code. However, I make
> "pci_enable_device" failed in vortext_up to let vortex_up return an
> error code directly, but "vp->rx_oom_timer" is initialized only by
> "init_timer" after "pci_enable_device". Thus when
> "del_timer_sync(&vp->rx_oom_timer);" is called in vortex_down,
> a null dereference may occur.
> Moreover, only "pci_enable_device" can make vortex_up failed.
> 
> 
Sooo, fix it.  Add some checks to not delete the timer if its not been
initalized.  Its really preferable to have a single teardown path and a single
bringup path if at all possible
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