Bug 237921
| Summary: | wpi: Memory leak in function wpi_free_tx_ring of sys/dev/wpi/if_wpi.c | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Young <yangx92> | ||||
| Component: | wireless | Assignee: | freebsd-wireless (Nobody) <wireless> | ||||
| Status: | Open --- | ||||||
| Severity: | Affects Some People | CC: | adrian, avos, benjsc, net, pauamma, wireless, yusisamerican | ||||
| Priority: | --- | Keywords: | needs-qa, patch | ||||
| Version: | CURRENT | Flags: | pauamma:
mfc-stable13?
koobs: mfc-stable12? |
||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
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 |
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; } }