On Fri, 19 Jun 2026 15:12:21 +0200
Morten Brørup <[email protected]> wrote:
> > + /*
> > + * Overlap with an existing fragment. Per RFC 8200 section
> > 4.5
> > + * (and RFC 5722) the datagram must be discarded; the same
> > is
> > + * applied to IPv4. Free all collected fragments, drop this
> > one,
> > + * and invalidate the entry.
> > + */
> > + if (ofs < fp->frags[i].ofs + fp->frags[i].len &&
> > + fp->frags[i].ofs < ofs + len) {
>
> This only catches fragments that are smaller than existing fragments, i.e.
> fit within one of the existing fragments.
> It should be:
> if ((ofs >= fp->frags[i].ofs &&
> ofs < fp->frags[i].ofs + fp->frags[i].len) ||
> (ofs + len >= fp->frags[i].ofs &&
> ofs + len < fp->frags[i].ofs + fp->frags[i].len)) {
>
> > + ip_frag_free(fp, dr);
The code here is comparing an incoming fragment N against existing fragment E,
using half-open ranges [start, end).
The test in the patch is symmetric in N and E.
ofs < e.ofs + e.len && e.ofs < ofs + len
The one you propose tests that either endpoint of N lands inside E.
Take a fixed stored fragment E = [200, 400) and run several incoming fragments
through both.
N0 = ofs, N1 = ofs+len.
N inside E: N = [250, 300)
E: |=========| (200..400)
N: |===| (250..300)
Patch: 250 < 400 && 200 < 300 → T && T → overlap.
Proposed: (250≥200 && 250<400) → T → overlap.
Both agree.
N encloses E: N = [100, 500)
E: |=========| (200..400)
N: |=============| (100..500)
Patch: 100 < 400 && 200 < 500 → T && T → overlap.
Proposed: (100≥200 && …) → F, (500≥200 && 500<400) → T && F → F, so F || F → no
overlap, MISSED.
This is the case the new version version drops. Neither endpoint of N (100 or
500) sits inside [200,400),
because N straddles E completely, so new version endpoint-in-E check fails even
though the ranges clearly overlap.
Patch version catches it because the interval test doesn't care which range is
larger.
N partial on the left: N = [100, 300)
E: |=========| (200..400)
N: |======| (100..300)
Patch: 100 < 400 && 200 < 300 → T → overlap.
Proposed: (300≥200 && 300<400) → T → overlap.
Agree.
N partial on the right: N = [300, 500) — symmetric to the above, both catch it.
So on the four genuine-overlap geometries, your suggestion catches all four and
his misses the enclosing one.
That is not right since the enclosing overlap is a legitimate attack shape (a
big fragment overwriting a smaller stored one).
There is another issue.
The >= on the exclusive end produces a false positive on fragments that merely
abut, which is the normal case.
Take E already stored as [1400, 2800) and an in-order-but-late fragment N = [0,
1400) arriving after it (ordinary out-of-order delivery):
N: |======| (0..1400)
E: |======| (1400..2800)
These share no bytes; byte 1400 belongs only to E.
Patch: 0 < 2800 && 1400 < 1400 → T && F → no overlap, correct.
Proposed: (1400≥1400 && 1400<2800) → T && T → overlap, wrong.
This test would discard a perfectly valid datagram whenever a left-abutting
fragment arrives after its neighbor.
Adjacent fragments abutting is what fragmentation produces by design, so this
would fire constantly under reordering.
Bottom line: the patch was correct as far as I can tell.
