* RE: DPDK ip_frag security analyis
[not found] <20260407172750.34e1aaf0@phoenix.local>
@ 2026-04-09 13:04 ` Konstantin Ananyev
2026-04-09 20:10 ` Stephen Hemminger
0 siblings, 1 reply; 2+ messages in thread
From: Konstantin Ananyev @ 2026-04-09 13:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev@dpdk.org
> Subject: Security analysis of lib/ip_frag (fragmentation & reassembly)
>
> Hi,
>
> Below is a security-focused review of the DPDK ip_frag library
> covering ip_frag_common.h, ip_frag_internal.c, ip_reassembly.h,
> rte_ip_frag.h, rte_ip_frag_common.c, rte_ipv4_fragmentation.c,
> rte_ipv4_reassembly.c, rte_ipv6_fragmentation.c, and
> rte_ipv6_reassembly.c.
>
> Findings are grouped by severity.
>
>
> ERRORS
> ======
>
> 1. TAILQ_FOREACH modification during iteration in
> rte_ip_frag_table_del_expired_entries()
>
> File: rte_ip_frag_common.c, lines 142-151
>
> TAILQ_FOREACH(fp, &tbl->lru, lru) iterates the LRU list while
> ip_frag_tbl_del() calls TAILQ_REMOVE() on the current element
> inside the loop body. TAILQ_FOREACH reads fp->lru.tqe_next in
> the loop-advance step *after* the body executes. After
> TAILQ_REMOVE, the removed element's tqe_next is stale.
>
> On glibc/BSD this happens to work because TAILQ_REMOVE does not
> zero tqe_next, but this violates the TAILQ API contract. If the
> implementation ever changes, or if a debug TAILQ that poisons
> removed entries is used, this becomes a use-after-remove bug.
>
> Fix: use TAILQ_FOREACH_SAFE, or save TAILQ_NEXT(fp, lru) before
> calling ip_frag_tbl_del().
ACK, that looks like a valid one to me.
>
> 2. No overlapping fragment detection (teardrop-class attacks,
> RFC 5722 violation for IPv6)
>
> File: ip_frag_internal.c, ip_frag_process()
>
> ip_frag_process() does not check whether a newly arrived fragment
> overlaps with any previously stored fragment. It only detects
> duplicate first/last fragments. For middle fragments, it blindly
> increments last_idx and stores the fragment.
>
> The reassembly functions happen to fail on overlapping fragments
> because the offset-matching chain walk cannot find a match, so
> the hole-detection fires. The mitigation is accidental rather
> than intentional.
Nope, it is not accidental, it was intentional design choice.
Checking for each incoming fragment that there is no overlaps might be
quite expensive. So, we postpone that check till we have all fragments
arrive and start re-assemble process.
It is sort of trade-off speed vs keeping mbuf with overlapping fragment
for a bit longer.
Looks like a false positive to me.
> For IPv6, RFC 5722 mandates that overlapping fragments MUST
> cause the entire datagram to be silently dropped. The current
> code does eventually drop them, but only after accumulating
> fragments and burning table slots until timeout. An attacker can
> exhaust fragment table entries by sending overlapping fragments.
>
> Fix: check new fragment offset/length against existing entries
> in frags[] before storing. For IPv6, drop immediately per
> RFC 5722.
>
>
> 3. Death row buffer overflow -- IP_FRAG_MBUF2DR has no bounds check
>
> File: ip_frag_common.h, line 38
>
> The IP_FRAG_MBUF2DR(dr, mb) macro writes to dr->row[dr->cnt++]
> with no bounds check. Similarly, ip_frag_free() adds up to
> IP_MAX_FRAG_NUM (8) entries without checking capacity.
>
> The death row is sized for RTE_IP_FRAG_DEATH_ROW_LEN *
> (MAX_FRAG + 1) = 288 entries. The del_expired_entries() function
> does check capacity, but the hot-path reassembly functions call
> ip_frag_find() -> ip_frag_tbl_del() and ip_frag_process() ->
> ip_frag_free() + IP_FRAG_MBUF2DR() without any capacity check.
>
> If an application calls reassembly in a tight loop without
> draining the death row between calls, dr->cnt can exceed
> RTE_IP_FRAG_DEATH_ROW_MBUF_LEN, causing an out-of-bounds write.
> The death row is typically stack-allocated, so this is a potential
> stack buffer overflow.
>
> ip_frag_find() can evict a stale entry AND an LRU entry in a
> single call (up to 16 mbufs), then ip_frag_process() can add 9
> more on error.A burst of 32 bad fragments hitting different
> stale entries could overflow.
That seems not entirely correct, as we just remove stale entry from
the table and the LRU list, so max IP_MAX_FRAG_NUM (8) can be deleted
in one go.
But overall, yes it is a potential problem, as we assume that user will
always operate in bursts of 32 packes, and call free_death_row()
after each such burst is processed.
Which might not be the always the case.
Needs to be addressed.
> Fix: add a bounds check in IP_FRAG_MBUF2DR and ip_frag_free(),
> or document the maximum number of reassembly calls permitted
> between death row drains.
>
>
> WARNINGS
> ========
>
> 4. IPv6 move_len underflow in ipv6_frag_reassemble()
>
> File: rte_ipv6_reassembly.c, line 104
>
> move_len = m->l2_len + m->l3_len - sizeof(*frag_hdr);
>
> move_len is uint32_t. If a buggy caller sets l2_len + l3_len < 8,
> move_len wraps to ~4 billion. The subsequent ip_frag_memmove()
> would attempt to copy gigabytes of data, causing a segfault or
> memory corruption.
>
> The API documents that callers must set l2_len/l3_len correctly,
> and for a valid IPv6 fragment l3_len alone should be >= 48 bytes.
> But there is no defensive validation.
>
> Fix: add a sanity check that l2_len + l3_len >= sizeof(*frag_hdr)
> before computing move_len.
I don't think we need to do anything here.
It should be safe to assume that input mbuf fields are valid,
specially if it is already documented.
>
>
> 5. rte_ipv6_frag_get_ipv6_fragment_header() only inspects the
> first extension header
>
> File: rte_ip_frag.h, line 143
>
> This function checks only hdr->proto == IPPROTO_FRAGMENT. If any
> other extension header (hop-by-hop, routing, destination options)
> precedes the fragment header, this function returns NULL and the
> packet is treated as non-fragmented.
>
> An attacker can evade reassembly by inserting a preceding
> extension header, causing fragments to be forwarded as-is. This
> can bypass firewall rules that rely on seeing reassembled traffic.
>
> The limitation is documented but is a security-relevant design
> gap for applications using this library.
Yep, it is a well-known and long-standing limitation that needs to be addressed.
I think it was even attempt to fix it, but for whatever reason it wasn't accepted.
> Fix: walk the extension header chain to find the fragment header,
> or clearly document the security implications in the API header.
>
>
> INFORMATIONAL
> =============
>
> 6. Hash collision DoS via fixed seed
>
> Both ipv4_frag_hash() and ipv6_frag_hash() use CRC32 (x86/ARM)
> or jhash with a fixed, publicly known prime seed (0xeaad8405).
> An attacker who can send crafted IP fragments can precompute hash
> collisions, causing all fragments to land in the same bucket.
> After bucket_entries concurrent flows collide, new flows are
> dropped.
>
> Fix: randomize the hash seed at table creation time.
ACK, seems valid - needs to be fixed.
>
> 7. max_cycles + fp->start wraparound
>
> The timeout comparison (max_cycles + fp->start < tms) at multiple
> locations uses uint64_t arithmetic. If the sum wraps past 2^64,
> entries appear timed out when they are not. With rdtsc-based
> timestamps this requires running for hundreds of years, so it is
> not practically exploitable. Worth noting for alternative time
> sources with faster-incrementing values.
False positive.
>
> 8. No total reassembled size limit before table insertion
>
> The library accepts and stores fragments up to IP_MAX_FRAG_NUM
> per datagram, but there is no explicit limit on total reassembled
> size. An attacker can claim total_size = 65535 via a crafted last
> fragment, then send middle fragments that consume table entries
> and death row capacity during the attack window, even though
> reassembly will ultimately fail when frag_size exceeds
> total_size.
We do have max number of frags perf packet.
Honestly I don't think there is a need for such extra check.
^ permalink raw reply [flat|nested] 2+ messages in thread