| Message ID | 20231207143822.3358727-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethernet: atheros: fix a memleak in atl1e_setup_ring_resources | expand |
>diff --git a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c >b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c >index 5935be190b9e..deb5a3f207cc 100644 >--- a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c >+++ b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c >@@ -866,6 +866,7 @@ static int atl1e_setup_ring_resources(struct >atl1e_adapter *adapter) > netdev_err(adapter->netdev, "offset(%d) > ring size(%d) !!\n", > offset, adapter->ring_size); > err = -1; >+ kfree(tx_ring->tx_buffer); [Suman] I think we should do tx_ring->tx_buffer = NULL also, to avoid use after free? > goto failed; > } > >-- >2.34.1 >
On Thu, 7 Dec 2023 17:08:15 +0000 Suman Ghosh wrote: > >+ kfree(tx_ring->tx_buffer); > > [Suman] I think we should do tx_ring->tx_buffer = NULL also, to avoid use after free? It's up to the driver. Some may call that defensive programming.
>On Thu, 7 Dec 2023 17:08:15 +0000 Suman Ghosh wrote: >> >+ kfree(tx_ring->tx_buffer); >> >> [Suman] I think we should do tx_ring->tx_buffer = NULL also, to avoid >use after free? > >It's up to the driver. Some may call that defensive programming. [Suman] Agree. I pointed it out since this driver is using this approach at other places. But sure, it is up to Zhipeng.
> >On Thu, 7 Dec 2023 17:08:15 +0000 Suman Ghosh wrote: > >> >+ kfree(tx_ring->tx_buffer); > >> > >> [Suman] I think we should do tx_ring->tx_buffer = NULL also, to avoid > >use after free? > > > >It's up to the driver. Some may call that defensive programming. > [Suman] Agree. I pointed it out since this driver is using this approach at other places. But sure, it is up to Zhipeng. [Zhipeng] I think Suman's suggestion is valuable, it prevents potiential use-after-free and is consistent with other free operations in the same module.
diff --git a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c index 5935be190b9e..deb5a3f207cc 100644 --- a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c +++ b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c @@ -866,6 +866,7 @@ static int atl1e_setup_ring_resources(struct atl1e_adapter *adapter) netdev_err(adapter->netdev, "offset(%d) > ring size(%d) !!\n", offset, adapter->ring_size); err = -1; + kfree(tx_ring->tx_buffer); goto failed; }
In the error handling of 'offset > adapter->ring_size', the tx_ring->tx_buffer allocated by kzalloc should be freed, instead of 'goto failed' instantly. Fixes: a6a5325239c2 ("atl1e: Atheros L1E Gigabit Ethernet driver") Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> --- drivers/net/ethernet/atheros/atl1e/atl1e_main.c | 1 + 1 file changed, 1 insertion(+)