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.
Details
Details
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 20729 Build 20139: arc lint + arc unit
Event Timeline
| sys/netinet/ip_reass.c | ||
|---|---|---|
| 209 | you could & IP_OFFMASK in host order | |
Comment Actions
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.) | |
Comment Actions
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. | |