Re: [PATCH 2/6] ip_frag: discard datagrams with overlapping fragments

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 19 Jun 2026 15:12:21 +0200
Morten Brørup <mb@smartsharesystems.com> 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.