[PATCH v2] net: pch_gbe: Fix memory leaks

[PATCH v2] net: pch_gbe: Fix memory leaks

[Wenwen Wang]

In pch_gbe_set_ringparam(), if netif_running() returns false, 'tx_old' and
'rx_old' are not deallocated, leading to memory leaks. To fix this issue,
move the free statements to the outside of the if() statement.

Signed-off-by: Wenwen Wang <wenwen@cs.uga.edu>
---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
index 1a3008e..cb43919 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
@@ -340,12 +340,10 @@ static int pch_gbe_set_ringparam(struct net_device *netdev,
 			goto err_setup_tx;
 		pch_gbe_free_rx_resources(adapter, rx_old);
 		pch_gbe_free_tx_resources(adapter, tx_old);
-		kfree(tx_old);
-		kfree(rx_old);
-		adapter->rx_ring = rxdr;
-		adapter->tx_ring = txdr;
 		err = pch_gbe_up(adapter);
 	}
+	kfree(tx_old);
+	kfree(rx_old);
 	return err;
 
 err_setup_tx:
-- 
2.7.4

Re: [PATCH v2] net: pch_gbe: Fix memory leaks

[David Miller]

From: Wenwen Wang <wenwen@cs.uga.edu>
Date: Tue, 20 Aug 2019 23:20:05 -0500

> In pch_gbe_set_ringparam(), if netif_running() returns false, 'tx_old' and
> 'rx_old' are not deallocated, leading to memory leaks. To fix this issue,
> move the free statements to the outside of the if() statement.
> 
> Signed-off-by: Wenwen Wang <wenwen@cs.uga.edu>

Something still is not right here.

> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
> index 1a3008e..cb43919 100644
> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_ethtool.c
> @@ -340,12 +340,10 @@ static int pch_gbe_set_ringparam(struct net_device *netdev,
>  			goto err_setup_tx;
>  		pch_gbe_free_rx_resources(adapter, rx_old);
>  		pch_gbe_free_tx_resources(adapter, tx_old);
> -		kfree(tx_old);
> -		kfree(rx_old);
> -		adapter->rx_ring = rxdr;
> -		adapter->tx_ring = txdr;
>  		err = pch_gbe_up(adapter);
>  	}
> +	kfree(tx_old);
> +	kfree(rx_old);

If the if() condition ending here is not taken, you cannot just free these
two pointers.  You are then leaking the memory which would normally be
liberated by pch_gbe_free_rx_resources() and pch_gbe_free_tx_resources().

What's more, in this same situation, the rx_old->dma value is probably still
programmed into the hardware, and therefore the device still could potentially
DMA read/write to that memory.

I think the fix here is not simple, and you will need to do more extensive
research in order to fix this properly.

I'm not applying this, sorry.