Re: [PATCH] netfilter: ECN target corrupts packets (2.6 only)

[Patrick McHardy <kaber@trash.net>]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

Maciej Soltysiak wrote:

>>Nasty bug, caught while writing the ECN target test.  Corrupts
>>checksums of packets when target is used on them.
>>    
>>
>>Let this be a warning on the evils of casts.
>>    
>>
>Heh, so that's why ECN stopped working for me :-)
>I will check that out.
>
>I did tcpdumps but for some strange reason I could not see
>the problem there. Maybe tcpdump got premangled packets..
>  
>
There's more brokeness, it mangles the tcp header before making
it writable and doesn't reload pointers after doing so. This patch
should fix all of it and as a side-effect avoids some useless
copying. Any voluteers for testing ? :)

Regards
Patrick


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1774 bytes --]

===== net/ipv4/netfilter/ipt_ECN.c 1.11 vs edited =====
--- 1.11/net/ipv4/netfilter/ipt_ECN.c	2004-10-26 01:10:25 +02:00
+++ edited/net/ipv4/netfilter/ipt_ECN.c	2005-01-02 22:00:14 +01:00
@@ -61,31 +61,28 @@
 	if (th == NULL)
 		return 0;
 
-	diffs[0] = ((u_int16_t *)th)[6];
-	if (einfo->operation & IPT_ECN_OP_SET_ECE)
-		th->ece = einfo->proto.tcp.ece;
-
-	if (einfo->operation & IPT_ECN_OP_SET_CWR)
-		th->cwr = einfo->proto.tcp.cwr;
-	diffs[1] = ((u_int16_t *)&th)[6];
-
-	/* Only mangle if it's changed. */
-	if (diffs[0] != diffs[1]) {
-		diffs[0] = diffs[0] ^ 0xFFFF;
+	if ((einfo->operation & IPT_ECN_OP_SET_ECE &&
+	     th->ece != einfo->proto.tcp.ece) ||
+	    (einfo->operation & IPT_ECN_OP_SET_CWR &&
+	     th->cwr != einfo->proto.tcp.cwr)) {
 		if (!skb_ip_make_writable(pskb,
 					  (*pskb)->nh.iph->ihl*4+sizeof(_tcph)))
 			return 0;
+		th = (void *)(*pskb)->nh.iph + (*pskb)->nh.iph->ihl*4;
 
-		if (th != &_tcph)
-			memcpy(&_tcph, th, sizeof(_tcph));
+		diffs[0] = ((u_int16_t *)th)[6];
+		if (einfo->operation & IPT_ECN_OP_SET_ECE)
+			th->ece = einfo->proto.tcp.ece;
+		if (einfo->operation & IPT_ECN_OP_SET_CWR)
+			th->cwr = einfo->proto.tcp.cwr;
+		diffs[1] = ((u_int16_t *)th)[6];
+		diffs[0] = diffs[0] ^ 0xFFFF;
 
 		if ((*pskb)->ip_summed != CHECKSUM_HW)
-			_tcph.check = csum_fold(csum_partial((char *)diffs,
-							     sizeof(diffs),
-							     _tcph.check^0xFFFF));
-		memcpy((*pskb)->data + (*pskb)->nh.iph->ihl*4,
-		       &_tcph, sizeof(_tcph));
-		if ((*pskb)->ip_summed == CHECKSUM_HW)
+			th->check = csum_fold(csum_partial((char *)diffs,
+			                      sizeof(diffs),
+			                      th->check^0xFFFF));
+		else
 			if (skb_checksum_help(*pskb, inward))
 				return 0;
 		(*pskb)->nfcache |= NFC_ALTERED;