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