memif driver segfault memory corruption.

memif driver segfault memory corruption.

[John Weston]

[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]

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@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

[-- Attachment #2: Type: text/html, Size: 18833 bytes --]

Re: memif driver segfault memory corruption.

[Ferruh Yigit]

On 7/16/2022 1:15 AM, John Weston wrote:
> 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@dpdk.org <mailto:dev@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

Hi John,

Thanks for reporting. Cc'ed memif maintainer.

Meanwhile, can you please submit a defect to bugzilla [1], to be sure 
issue is recorded properly?

Also if you want to contribute the fix yourself, please check the 
contribution guide:
https://doc.dpdk.org/guides/contributing/patches.html


[1]
https://bugs.dpdk.org/