memif driver segfault memory corruption.

[John Weston]

Hi,

I have been working on the dpdk memif driver and encountered the following
issues. Please consider these patches for review.

Firstly, the zero copy code in drivers/net/memif/rte_eth_memif.c can
readilly overflow the ring buffer for mbufs. This patch adjusts the n_slots
used in rte_poktmbuf_alloc_bulk to ensure it does not run off the end of
the buffers list.

dev at dpdk.org@@ -524,14 +1196,23 @@ refill:

         */

        head = __atomic_load_n(&ring->head, __ATOMIC_RELAXED);

        n_slots = ring_size - head + mq->last_tail;

-

-       if (n_slots < 32)

-               goto no_free_mbufs;

+    // CTAM - critical BUG FIX !

+#if 1

+    // only work within mask so alloc_bulk does not overrun the buffers
array

+    if ((head&mask) + n_slots > ring_size)

+    {

+        n_slots = ring_size - (head&mask);

+    }

+    if (n_slots <=0)

+         goto no_free_mbufs;

+#else

+    if (n_slots < 32)

+         goto no_free_mbufs;

+#endif


        ret = rte_pktmbuf_alloc_bulk(mq->mempool, &mq->buffers[head &
mask], n_slots);

        if (unlikely(ret < 0))

                goto no_free_mbufs;

-

        while (n_slots--) {

                s0 = head++ & mask;

                if (n_slots > 0)




Secondly, a segfault can occur on the stats routine on initialisation due
to null pointer dereferencing.


@@ -1404,10 +2167,14 @@ memif_stats_get(struct rte_eth_dev *dev, struct
rte_eth_stats *stats)

        /* RX stats */

        for (i = 0; i < nq; i++) {

                mq = dev->data->rx_queues[i];

-               stats->q_ipackets[i] = mq->n_pkts;

-               stats->q_ibytes[i] = mq->n_bytes;

-               stats->ipackets += mq->n_pkts;

-               stats->ibytes += mq->n_bytes;

+        // CTAM test this, as it may not yet initialised!

+        if (mq)

+        {

+                   stats->q_ipackets[i] = mq->n_pkts;

+                   stats->q_ibytes[i] = mq->n_bytes;

+                   stats->ipackets += mq->n_pkts;

+                   stats->ibytes += mq->n_bytes;

+        }

        }


        tmp = (pmd->role == MEMIF_ROLE_CLIENT) ? pmd->run.num_s2c_rings :


This can occur in several places in the stats code, and null dereference
guards are needed in all locations.


Hope this helps someone else.


John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mails.dpdk.org/archives/dev/attachments/20220716/4946c4f8/attachment.htm>