2019-08-21 05:26:53

by Wenwen Wang

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

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 <[email protected]>
---
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

2019-08-23 08:56:02

by David Miller

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

From: Wenwen Wang <[email protected]>
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 <[email protected]>

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.