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

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Friday, 19 June 2026 19.01
> 
> On Fri, 19 Jun 2026 15:12:21 +0200
> Morten Brørup <mb at 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.

Thank you for the detailed explanation, Stephen.
Agreed, and sorry about the noise. :-)