Check for an overly large fragment.

[markj]

Summary

Packets exceeding this limit can trigger an overflow in the code which
inserts the fragment into its list; this can be used to violate the
invariant that fragments are sorted by increasing ip_off.

[emaste]

sys/netinet/ip_reass.c

209

you could & IP_OFFMASK in host order

[jtl]

Yeah, I saw this shortly before seeing this review. I agree we should fix it.

sys/netinet/ip_reass.c

209

We really should convert all of these to host order once on receipt and then just leave them that way until we are ready to reassemble. (I'm not suggesting we make that part of the same commit, but its yet one more piece of cleanup.)

209

Have you confirmed that this comparison will be promoted to something wider than 16 bits by the compiler? (In other words, will the comparison itself be safe from overflow on all of our compilers?) I don't see anything in here that is specifically wider than 16 bits that would trigger that.

209

To be pedantic, this could do "the wrong thing" if the IP header of the first fragment had no options, but subsequent fragments did have options. With this check, you could end up discarding a fragment (which came with options) at the end of the packet, even though that amount of data would fit in the reassembled IP packet (if the first fragment had no options).

You could avoid this by moving the check below the place where we adjust ip_len.

(Note that I personally think such a scenario is both highly unusual and broken.)

[markj]

I'm writing a regression test for this to stick under tests/sys/netinet. I'll see if I can add test cases for D17922 as well.

sys/netinet/ip_reass.c

209

I agree that we could use some cleanup here, I just didn't want to pollute the review.

Regarding type widths, yes, everything should be promoted to int before the comparison. We depend on this on line 368 as well.

I'll move the check below the adjustment.

[jtl]

In D17914#382797, @markj wrote:

I'm writing a regression test for this to stick under tests/sys/netinet. I'll see if I can add test cases for D17922 as well.

Thanks! That's great; it will be very helpful!

[bz]

Sorry; getting IPv4 fragments into my head is not a good idea.

[markj]

• Address review feedback.