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

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 22 Jun 2026 17:01:18 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Friday, 19 June 2026 19.01
> > 
> > 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.  
> 
> Thank you for the detailed explanation, Stephen.
> Agreed, and sorry about the noise. :-)
> 

I will give credit to Claude for the detail. I reviewed the general
code here; but had to prod it into giving a more detailed explaination
because it was confusing..