>Number:         5380
>Category:       kern
>Synopsis:       bad incremental checksum fixup in netinet/ip_flow.c
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people (Kernel Bug People)
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Apr 30 21:05:00 1998
>Last-Modified:
>Originator:     Dennis Ferguson
>Organization:
Juniper Networks
>Release:        NetBSD-current April 30 1998
>Environment:

>Description:
In netinet/ip_flow.c, the following code fragment is used to fix up
the header checksum after a ttl decrement:

	ip->ip_ttl -= IPTTLDEC;
#if BYTE_ORDER == LITTLE_ENDIAN
	sum = ip->ip_sum + IPTTLDEC;
#endif
#if BYTE_ORDER == BIG_ENDIAN
	sum = ip->ip_sum + (IPTTLDEC << 8);
	sum = (sum & 0xFFFF) + (sum >> 16);
#endif
	if (sum > 0x10000)		/* add in carry if needed */
		sum++;
	ip->ip_sum = sum;		/* bit 16 is dropped */

This is bad in a number of ways.  Assuming IPTTLDEC is 1 (though
the problems are independent of this):

(1) If the incoming checksum is 0xfffe on a little endian machine,
    or 0xfeff on a big endian machine, the code will generate a
    checksum of 0xffff.  While this is not strictly incorrect, note
    that in_cksum() always avoids this and will always produce the
    equivalent 0x0000 in this case.  The reason it is poor form to
    generate a 0xffff header checksum is that it can cause systems
    which do the header checksum check by recomputing it and comparing
    to the value in the header to fail.

(2) If the incoming checksum is 0xffff the updated checksum will be
    incorrect in the little endian case.  The updated checksum will
    be 0x0000, when it should be 0x0001.

(3) The `if (sum > 0x10000) sum++;' does nothing in the big endian
    case and fails to prevent incorrect computations in the little
    endian case.

(4) The big endian case compiles to more instructions than it needs
    to.

(5) The #if's are unnecessary given an htons() macro which compiles
    to a constant when called with a constant argument.

The same problems exist for IPTTLDEC other than 1, only the numbers
change.


>How-To-Repeat:

>Fix:
Given an htons() macro which compiles to a constant with a constant
argument the following code fragment avoids generating a 0xffff
checksum, produces a correct checksum in all cases, avoids the #if's,
and does the operation with a single comparison and addition on
machines of either byte order.

	ip->ip_ttl -= IPTTLDEC;
	if (ip->ip_sum >= htons(0xffff - (IPTTLDEC << 8))) {
		ip->ip_sum += htons(IPTTLDEC << 8) + 1;
	} else {
		ip->ip_sum += htons(IPTTLDEC << 8);
	}

(The same technique can be used to update the ICMP checksum in
an ICMP echo reply, with IPTTLDEC replaced by 3).
>Audit-Trail:
>Unformatted: