There is a memory leak and possible use-after-free vulnerability in function wpi_free_tx_ring of sys/dev/wpi/if_wpi.c. static void wpi_free_tx_ring(struct wpi_softc *sc, struct wpi_tx_ring *ring) { int i; DPRINTF(sc, WPI_DEBUG_TRACE, TRACE_STR_DOING, __func__); wpi_dma_contig_free(&ring->desc_dma); wpi_dma_contig_free(&ring->cmd_dma); for (i = 0; i < WPI_TX_RING_COUNT; i++) { struct wpi_tx_data *data = &ring->data[i]; if (data->m != NULL) { bus_dmamap_sync(ring->data_dmat, data->map, BUS_DMASYNC_POSTWRITE); bus_dmamap_unload(ring->data_dmat, data->map); m_freem(data->m); } if (data->map != NULL) bus_dmamap_destroy(ring->data_dmat, data->map); } if (ring->data_dmat != NULL) { bus_dma_tag_destroy(ring->data_dmat); ring->data_dmat = NULL; } } There are two pointers in struct wpi_tx_data. struct wpi_tx_data { bus_dmamap_t map; bus_addr_t cmd_paddr; struct mbuf *m; struct ieee80211_node *ni; int hdrlen; }; In the for-loop, the function should both free data->m and data->ni. Besides, there should be data->m = NULL; after m_freem(data->m). Otherwise, there may be use-after-free vulnerability. Below are proposed patch. static void wpi_free_tx_ring(struct wpi_softc *sc, struct wpi_tx_ring *ring) { int i; DPRINTF(sc, WPI_DEBUG_TRACE, TRACE_STR_DOING, __func__); wpi_dma_contig_free(&ring->desc_dma); wpi_dma_contig_free(&ring->cmd_dma); for (i = 0; i < WPI_TX_RING_COUNT; i++) { struct wpi_tx_data *data = &ring->data[i]; if (data->m != NULL) { bus_dmamap_sync(ring->data_dmat, data->map, BUS_DMASYNC_POSTWRITE); bus_dmamap_unload(ring->data_dmat, data->map); m_freem(data->m); + data->m = NULL; } if (data->map != NULL) bus_dmamap_destroy(ring->data_dmat, data->map); + if (data->ni != NULL) { + ieee80211_free_node(data->ni); + data->ni = NULL; + } } if (ring->data_dmat != NULL) { bus_dma_tag_destroy(ring->data_dmat); ring->data_dmat = NULL; } }
Thank you for your quality report. Could you please include your proposed patch an attachment please?
Created attachment 204398 [details] Proposed patch
A quick search through that source file shows wpi_reset_tx_ring does that but wpi_free_tx_ring doesn't. Should it? (Note: patch may need refreshing.) Reminded-of-by: Yusuf Khan <yusisamerican at gmail dot com> in Libera #freebsd
(In reply to PauAmma from comment #3) I believe that is the point of the patch, to bring over that patch designed for wpi_reset_tx_ring to wpi_free_tx_ring
(In reply to Yusuf Khan from comment #4) check* not patch
This getting merged? Its been 5 years now
(In reply to Yusuf Khan from comment #6) wpi -- Intel 3945ABG Wireless LAN IEEE 802.11 driver I would like to help with the patch merge, but I don't have the device to test. Yusuf, are you able to help test it?
(In reply to Cheng Cui from comment #7) I dont have the device to test it either...It looks good to me anyway.
(In reply to Yusuf Khan from comment #8) I cannot follow the logic. wpi_free_tx_ring() is called from device detach (ignoring the attach error path); the mbuf/ring descriptor will not be used beyond that function anymore at that stage so there is no need to set it to NULL anymore. That would just be a NOP. If there's still a reference to a ni at that stage, then other cleanup hasn't worked at the beginning of wpi_detach(). There should be no more ni; in fact that should also be no more mbuf attached on the TX ring so I would even question if that code path is ever taken. This function is basically doing the opposite of wpi_alloc_tx_ring(). wpi_reset_tx_ring() on the contrary leaves all resources in place and only cleans up any outstanding transations and resets the ring to default values; this happens in wpi_hw_stop(). And then the resources will be re-used again after wpi_parent() goes through the bringup part again (a VAP/wlan interface is brought up again). So let me ask what lead to the original assumption that there may be a memory leak and a possible use-after-free. If you indeed believe that, I would love to know the code path which can trigger it.