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

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

[Rusty Russell]

Name: ipt_ECN corrupt checksum fix
Status: Tested under nfsim
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

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.

Index: linux-2.6.10-bk1-Netfilter/net/ipv4/netfilter/ipt_ECN.c
===================================================================
--- linux-2.6.10-bk1-Netfilter.orig/net/ipv4/netfilter/ipt_ECN.c	2005-01-02 21:47:31.439866944 +1100
+++ linux-2.6.10-bk1-Netfilter/net/ipv4/netfilter/ipt_ECN.c	2005-01-02 21:56:47.396348712 +1100
@@ -67,7 +67,7 @@
 
 	if (einfo->operation & IPT_ECN_OP_SET_CWR)
 		th->cwr = einfo->proto.tcp.cwr;
-	diffs[1] = ((u_int16_t *)&th)[6];
+	diffs[1] = ((u_int16_t *)th)[6];
 
 	/* Only mangle if it's changed. */
 	if (diffs[0] != diffs[1]) {

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

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

[Maciej Soltysiak]

Hello Rusty,

Sunday, January 2, 2005, 12:17:40 PM, you wrote:

> Name: ipt_ECN corrupt checksum fix
> Status: Tested under nfsim
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

> 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..

Regards,
Maciej

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

[Patrick McHardy]

[-- 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;

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

[Rusty Russell]

On Sun, 2005-01-02 at 22:04 +0100, Patrick McHardy wrote:
> 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 ? :)

I see more nfsim work coming up...

Thanks!
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman