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: kernAssignee: freebsd-wireless mailing list <wireless>
Status: New ---    
Severity: Affects Some People CC: adrian, avos, benjsc, net, wireless
Priority: --- Keywords: needs-qa
Version: CURRENTFlags: koobs: mfc-stable12?
koobs: mfc-stable11?
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Proposed patch none

Description Young 2019-05-16 11:50:17 UTC
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;
        }
}
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-05-16 12:01:31 UTC
Thank you for your quality report. Could you please include your proposed patch an attachment please?
Comment 2 Young 2019-05-16 12:06:46 UTC
Created attachment 204398 [details]
Proposed patch