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]

Message-ID: <20141223142439.GD31876@hmsreliant.think-freely.org>
Date:	Tue, 23 Dec 2014 09:24:39 -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 10:54:50AM +0800, Jia-Ju Bai wrote:
> For linux-3.18.0
> The driver calls __netdev_alloc_skb in vortex_open but lacks dev_kfree_skb
> when vortex_up is failed, so memory leaks may occur in this situation.
> This patch fixes this problem.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@....com>
> ---
>  drivers/net/ethernet/3com/3c59x.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
> index 41095eb..d0c5bee 100644
> --- a/drivers/net/ethernet/3com/3c59x.c
> +++ b/drivers/net/ethernet/3com/3c59x.c
> @@ -1782,6 +1782,16 @@ vortex_open(struct net_device *dev)
>  	if (!retval)
>  		goto out;
>  
> +	if (vp->full_bus_master_rx) {
> +		for (i = 0; i < RX_RING_SIZE; i++) {
> +			if (vp->rx_skbuff[i]) {
> +				dev_kfree_skb(vp->rx_skbuff[i]);
> +				vp->rx_skbuff[i] = NULL;
> +			}
> +		}
> +		retval = -ENOMEM;
> +	}
> +
>  err_free_irq:
>  	free_irq(dev->irq, dev);
>  err:
> -- 
> 1.7.9.5
> 
> 
> 
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.

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

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