On Mon, 22 Jun 2026 17:01:18 +0200 Morten Brørup <[email protected]> wrote:
> > 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. :-) > 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..
