fix use after free issue in mbuf free

[tanjianfeng]

Two kinds of mbuf are used in f-stack: freebsd mbuf and dpdk mbuf.

freebsd mbufs are metadata used in freebsd stack, and their data
pointers (m_data) point to dpdk mbuf's data (buf_addr). And they have
their own chain, like this:

  bsd_mbuf1 -> bsd_mbuf2 -> bsd_mbuf3
      \            \           \
    dpdk_mbuf1 -> dpdk_mbuf2 -> dpdk_mbuf3

Considering the map relationship,

• m_freem() is corresponding to rte_pktmbuf_free(), is to free the whole
  chain of mbufs.
• m_free() is corresponding to rte_pktmbuf_free_seg(), is to free the
  specified mbuf segment.

The current implementation in f-stack uses rte_pktmbuf_free() for
m_free(). This leads to mbufs, which are still in use, be freed
unexpectedly. For example, if the bsd_mbuf1 is trimed into zero length,
bsd will invoke m_free() to free the specified segment, however, the
whole mbuf chain is freed by calling rte_pktmbuf_free().

#0 rte_pktmbuf_free (m=0x22006fb480)
#1 in ff_dpdk_pktmbuf_free (m=0x22006fb480)
#2 in ff_mbuf_ext_free (m=0x7ffff7f82800, arg1=0x22006fb480, arg2=0x0)
#3 in mb_free_ext (m=0x7ffff7f82800)
#4 in m_free (m=0x7ffff7f82800)
#5 in sbcompress (sb=, m=0x7ffff7f82800, n=)
#6 in sbappendstream_locked (sb=, m=0x7ffff7f82800, flags=0)

The fix is straightforward. Use the correct API for segment free.

Reported-by: Yong-Hao Zou yonghaoz1994@gmail.com
Signed-off-by: Jianfeng Tan henry.tjf@antgroup.com

[freak82]

Hi there,

I could be wrong but from the code inside the ff_veth_input function it seems to me that a rte_mbuf is attached only to the main bsd mbuf (ff_mbuf_gethdr) and the rte_mbuf segments are not attached to their corresponding bsd mbuf segments (ff_mbuf_get).

void *hdr = ff_mbuf_gethdr(pkt, pkt->pkt_len, data, len, rx_csum);
    if (hdr == NULL) {
        rte_pktmbuf_free(pkt);
        return;
    }

    if (pkt->ol_flags & PKT_RX_VLAN_STRIPPED) {
        ff_mbuf_set_vlan_info(hdr, pkt->vlan_tci);
    }

    struct rte_mbuf *pn = pkt->next;
    void *prev = hdr;
    while(pn != NULL) {
        data = rte_pktmbuf_mtod(pn, void*);
        len = rte_pktmbuf_data_len(pn);

        void *mb = ff_mbuf_get(prev, data, len);
        if (mb == NULL) {
            ff_mbuf_free(hdr);
            rte_pktmbuf_free(pkt);
            return;
        }
        pn = pn->next;
        prev = mb;
    }

From the above code snippet, it seems to me that the relation between the buffers is:

bsd_mbuf1 -> bsd_mbuf2 -> bsd_mbuf3 -> ...
    \
dpdk_mbuf1 -> dpdk_mbuf2 -> dpdk_mbuf3 -> ...

I could be wrong though. Let's double check it.

Pavel.

[zouyonghao]

This is related to #556

[tanjianfeng]

It's chained segment by segment, isn't it?

while(pn != NULL) {
    data = rte_pktmbuf_mtod(pn, void*);
    len = rte_pktmbuf_data_len(pn);

    void *mb = ff_mbuf_get(prev, data, len);
    if (mb == NULL) {
        ff_mbuf_free(hdr);
        rte_pktmbuf_free(pkt);
        return;
    }
    pn = pn->next;
    prev = mb;
}

[freak82]

I think, the data of the segments is chained.
The ff_mbuf_get function links the data to the BSD mbuf because it does m_extadd(mb, data, len, NULL, NULL, NULL, 0, 0); but it passes NULL for freef function and NULL for the arguments to this function.
However, the ff_mbuf_gethdr not only links the data but it also provides function for freeing the packet and the DPDK packet itself - m_extadd(m, data, len, ff_mbuf_ext_free, pkt, NULL, 0, EXT_DISPOSABLE);
As far as I understand the code there, the ff_mbuf_gethdr is called only for the header packet and not for the following segments. So I think that the ff_mbuf_ext_free is called only when the header packet (BSD mbuf) is freed and not for the segments.

[tanjianfeng]

So I think that the ff_mbuf_ext_free is called only when the header packet (BSD mbuf) is freed and not for the segments.

That's exactly the case we encounter. When bsd stack intput this packet chain, it finds that the part pointed by bsd_mbuf1 is already recevied by previous packet, so it sets bsd_mbufs1->m_len = 0; and before it puts later segments into receive buffer, it invokes sbcompress() to free those segments with m_len = 0.

That is to say, bsd wants to free the first segment, instead of the whole chain. But orginal implementation would the whole chain; as a result dpdk_mbuf2 and dpdk_mbuf3 are freed and reused while they are still in receive buffer.

  bsd_mbuf1 -> bsd_mbuf2 -> bsd_mbuf3
      \            \           \
    dpdk_mbuf1 -> dpdk_mbuf2 -> dpdk_mbuf3

[tanjianfeng]

BTW, this bug can be expoited for DOS attack; and what's worse, it could be reused to deliver more serious attack.

[freak82]

As far as I understand the proposed fix, it's going to free only the first segment of the DPDK mbuf and then the next segments won't be freed because they haven't been attached to the BSD segments. The BSD segments only point to their data.
It seems to me that the segments will leak in this case?
Shouldn't the fix change the code so that it attaches every DPDK mbuf to the corresponding BSD segment via m_extadd(m, data, len, ff_mbuf_ext_free, pkt, NULL, 0, EXT_DISPOSABLE)?
Maybe the ff_mbuf_get function should acall
m_extadd(m, data, len, ff_mbuf_ext_free, pkt, NULL, 0, EXT_DISPOSABLE)
instead of
m_extadd(mb, data, len, NULL, NULL, NULL, 0, 0);.
As far as I checked the ff_mbuf_get function is called only in the loop that you posted above. So, no other code will be affected.
This with the addition of your change should fix the mentioned use after free allowing the fragmented packets to be freed segment by segment.

[tanjianfeng]

As far as I understand the proposed fix, it's going to free only the first segment of the DPDK mbuf and then the next segments won't be freed because they haven't been attached to the BSD segments. The BSD segments only point to their data.

Yep.

It seems to me that the segments will leak in this case?
Shouldn't the fix change the code so that it attaches every DPDK mbuf to the corresponding BSD segment via m_extadd(m, data, len, ff_mbuf_ext_free, pkt, NULL, 0, EXT_DISPOSABLE)?

Make sense. I'll try your suggestion, and do some local test to make sure those dpdk mbufs are not leaked. Thanks!

[tanjianfeng]

@freak82 Changed as you suggested, and I wrote a simple test to make sure that every segment is freed (unfortunately, I did not find a way to share the test case).

[zouyonghao]

Can @jfb8856606 confirm this?