svn commit: r280971 - in head: contrib/ipfilter/tools share/man/man4 sys/contrib/ipfilter/netinet sys/netinet sys/netipsec sys/netpfil/pf

[Robert N. M. Watson]

On 2 Apr 2015, at 21:54, Hans Petter Selasky <hps at selasky.org> wrote:

>>> Please go read about how IP fragmentation works.  Having an identical IP
>>> ID in ip_fragment() is the point of the function!
>> 
>> rwatson: You're right, the more fragment flag gets set there, I
>> overlooked that bit. Sorry.
>> 
>> glebius: Given that you admit there is a small chance of an IP ID
>> collision in the previous e-mails exchanged in this thread, why don't we
>> have checks for that in ip_reass() when receiving fragmented IP packets?
>> For example when ip->ip_off == 0 we know the TCP and/or UDP port numbers
>> for TCP and UDP payloads and can check if a new fragment is starting
>> before the previous one is completed. Then we would know if a collision
>> has happened and could discard that packet. Not ideal, but better than
>> data corruption.
> 
> I see from the code that if two frags have the same IP offset, the whole fragment list gets dropped, unless the IP payload is zero bytes long. Maybe a "last" variable should be added?

Are you solving an actual problem you've observed, or is this a speculative proposal? RFC 791 requires that the minimum fragment size be 8 octets, and ip_input() drops fragments below that size, so you shouldn't (in principle) be able to get a fragment in the reassembly code that has the properties you've described. If you are, it's a bug elsewhere in the stack. If you are worried, you could add an assertion at the top of the reassembly function that the size is >= 8 octets.

I think you would benefit a great deal from reading Stevens Volume I (second edition) before proceeding with further changes to the TCP/IP code stack. A number of the proposals you have made are contradictory to fundamental design choices in the protocol. Although there isn't a second edition of Volume II, it might still be a good idea to skim through the pertinent sections there as well -- it applies to a much (much) earlier version of the stack, but includes a detailed exposition of how the implementation tracks the protocol.

Robert

>>         * only n will ever be stored. (n = maxfragsperpacket.)
>>         *
>>         */
>>        next = 0;
>          last = -1;
>>        for (p = NULL, q = fp->ipq_frags; q; p = q, q = q->m_nextpkt) {
>>                if (ntohs(GETIP(q)->ip_off) != next  ||
> +			ntohs(GETIP(q)->ip_off) == last
>> 			) {
>>                        if (fp->ipq_nfrags > V_maxfragsperpacket) {
>>                                IPSTAT_ADD(ips_fragdropped, fp->ipq_nfrags);
>>                                ip_freef(head, fp);
>>                        }
>>                        goto done;
>>                }
> 		  last = next;
>>                next += ntohs(GETIP(q)->ip_len);
>>        }
> 
> --HPS
>