Re: [PATCH v2] ip_frag: support IPv6 reassembly with extensions

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 15 Oct 2024 10:21:33 +0200
<vignesh.purushotham.srinivas@ericsson.com> wrote:

> From: Vignesh PS <vignesh.purushotham.srinivas@ericsson.com>
> 
> Add support to ip_frag library to perform IPv6 reassembly
> when extension headers are present before the fragment
> extension in the packet.
> 
> Signed-off-by: Vignesh PS <vignesh.purushotham.srinivas@ericsson.com>
> ---
>  .mailmap                          |   1 +
>  app/test/test_reassembly_perf.c   | 163 +++++++++++++++++++-----------
>  lib/ip_frag/ip_frag_common.h      |   4 +
>  lib/ip_frag/ip_reassembly.h       |   2 +
>  lib/ip_frag/rte_ipv6_reassembly.c |  75 ++++++++++++--
>  5 files changed, 179 insertions(+), 66 deletions(-)


This patch was never reviewed in detail.
AI review found some issues, it would need changes.

## Patch Feedback Summary

### Critical Bug — Fix Required

**NULL dereference when first fragment arrives last** (`rte_ipv6_reassembly.c`)

The code sets `fp->next_proto` *after* calling `ip_frag_process()`, but `ip_frag_process()` can immediately trigger `ipv6_frag_reassemble()` before returning — which dereferences `fp->next_proto`. Since `ip_frag_reset()` initializes it to NULL, any flow where the first fragment arrives last will crash.

**Fix**: Move the `fp->next_proto` / `fp->exts_len` assignment block to *before* the `ip_frag_process()` call.

---

### Security Bug — Fix Required

**No bounds check in `ip_frag_get_last_exthdr()`** (`rte_ipv6_reassembly.c`)

The loop advances through extension headers using `ext_len` values read from packet data, with no check that the accumulated `total_len` stays within the packet's actual payload length. A crafted packet with malformed extension headers can walk the pointer off the end of the mbuf's data buffer (out-of-bounds read). Add a check that `total_len + ext_len <= ip_hdr->payload_len` on each iteration.

---

### Minor Issues

- **Typo in comment**: `"or th next header"` → `"or the next header"`
- **`ip_frag_get_last_exthdr` return type**: Returns `int` but accumulates into `uint32_t`; large crafted extension stacks could produce a false negative error return. Consider returning `int32_t` with a documented cap, or restructuring to use an out-parameter.