[PATCH] xx_nat_proto_gre: gre_key returns wrong pointer

[PATCH] xx_nat_proto_gre: gre_key returns wrong pointer

[Jorge Boncompte [DTI2]]

    Hi,

    While porting some changes of the 2.6.21-rc7 pptp/proto_gre conntrack 
and nat modules to a 2.4.32 kernel I noticed that the gre_key function 
returns a wrong pointer to the GRE key of a version 0 packet. I fixed it the 
way below but I don't know if it is right way. The gre_csum function seems 
to have the same problem. If the problem is real and the solution right I 
can resend with a signed-off line.

---  
linux-2.6.21-rc7.orig/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h 
2007-04-19 17:54:26.000000000 +0200
+++ linux-2.6.21-rc7/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h 
2007-04-19 17:55:11.000000000 +0200
@@ -97,8 +97,8 @@
  if (!greh->key)
   return NULL;
  if (greh->csum || greh->routing)
-  return (__be32 *) (greh+sizeof(*greh)+4);
- return (__be32 *) (greh+sizeof(*greh));
+  return (__be32 *) ((void *)greh+sizeof(*greh)+4);
+ return (__be32 *) ((void *)greh+sizeof(*greh));
 }

 /* get pointer ot gre csum, if present */
@@ -106,7 +106,7 @@
 {
  if (!greh->csum)
   return NULL;
- return (__sum16 *) (greh+sizeof(*greh));
+ return (__sum16 *) ((void *)greh+sizeof(*greh));
 }

 #endif /* __KERNEL__ */

    -Jorge

==============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5   14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- Sin pistachos no hay Rock & Roll...
- Without wicker a basket cannot be made.
==============================================================

Re: [PATCH] xx_nat_proto_gre: gre_key returns wrong pointer

[Patrick McHardy]

Jorge Boncompte [DTI2] wrote:
>    While porting some changes of the 2.6.21-rc7 pptp/proto_gre conntrack
> and nat modules to a 2.4.32 kernel I noticed that the gre_key function
> returns a wrong pointer to the GRE key of a version 0 packet. I fixed it
> the way below but I don't know if it is right way. The gre_csum function
> seems to have the same problem. If the problem is real and the solution
> right I can resend with a signed-off line.


Good catch. I think what it should do is not touch the key/checksum of
version 0 GRE packets at all since the intention is to behave identical
to nf_conntrack_proto_generic/nf_nat_proto_unknown in that case.

Its a bit more complicated to do this because we also need to avoid
altering the tuple in gre_unique_tuple. If you want to give it a try
please go ahead, otherwise I can look into it.

Re: [PATCH] xx_nat_proto_gre: gre_key returns wrong pointer

[Jorge Boncompte [DTI2]]

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

----- Original Message ----- 
From: "Patrick McHardy" <kaber@trash.net>
To: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Cc: <netfilter-devel@lists.netfilter.org>
Sent: Tuesday, April 24, 2007 2:50 PM
Subject: Re: [PATCH] xx_nat_proto_gre: gre_key returns wrong pointer


> Good catch. I think what it should do is not touch the key/checksum of
> version 0 GRE packets at all since the intention is to behave identical
> to nf_conntrack_proto_generic/nf_nat_proto_unknown in that case.
>
> Its a bit more complicated to do this because we also need to avoid
> altering the tuple in gre_unique_tuple. If you want to give it a try
> please go ahead, otherwise I can look into it.

    Hi, Patrick take a look at this patch when you have time please. I
tested it and behaves correctly, I can run one and only one GRE tunnel
though a NATing box and PPTP still works too, if you try with a second
tunnel from different host by the fact of not modifying the tuple it does
not work as the comment in get_unique_tuple says.


diff -uNr linux-2.6.21.orig/include/linux/netfilter/nf_conntrack_proto_gre.h 
linux-2.6.21/include/linux/netfilter/nf_conntrack_proto_gre.h
--- linux-2.6.21.orig/include/linux/netfilter/nf_conntrack_proto_gre.h 
2007-04-26 18:10:56.000000000 +0200
+++ linux-2.6.21/include/linux/netfilter/nf_conntrack_proto_gre.h 2007-04-26 
18:53:58.000000000 +0200
@@ -87,24 +87,6 @@
 /* delete keymap entries */
 void nf_ct_gre_keymap_destroy(struct nf_conn *ct);

-/* get pointer to gre key, if present */
-static inline __be32 *gre_key(struct gre_hdr *greh)
-{
- if (!greh->key)
-  return NULL;
- if (greh->csum || greh->routing)
-  return (__be32 *)(greh+sizeof(*greh)+4);
- return (__be32 *)(greh+sizeof(*greh));
-}
-
-/* get pointer ot gre csum, if present */
-static inline __sum16 *gre_csum(struct gre_hdr *greh)
-{
- if (!greh->csum)
-  return NULL;
- return (__sum16 *)(greh+sizeof(*greh));
-}
-
 extern void nf_ct_gre_keymap_flush(void);
 extern void nf_nat_need_gre(void);

diff -uNr 
linux-2.6.21.orig/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h 
linux-2.6.21/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h
--- linux-2.6.21.orig/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h 
2007-04-26 18:10:56.000000000 +0200
+++ linux-2.6.21/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h 
2007-04-26 18:54:39.000000000 +0200
@@ -90,25 +90,6 @@
 /* delete keymap entries */
 void ip_ct_gre_keymap_destroy(struct ip_conntrack *ct);

-
-/* get pointer to gre key, if present */
-static inline __be32 *gre_key(struct gre_hdr *greh)
-{
- if (!greh->key)
-  return NULL;
- if (greh->csum || greh->routing)
-  return (__be32 *) (greh+sizeof(*greh)+4);
- return (__be32 *) (greh+sizeof(*greh));
-}
-
-/* get pointer ot gre csum, if present */
-static inline __sum16 *gre_csum(struct gre_hdr *greh)
-{
- if (!greh->csum)
-  return NULL;
- return (__sum16 *) (greh+sizeof(*greh));
-}
-
 #endif /* __KERNEL__ */

 #endif /* _CONNTRACK_PROTO_GRE_H */
diff -uNr linux-2.6.21.orig/net/ipv4/netfilter/ip_nat_proto_gre.c 
linux-2.6.21/net/ipv4/netfilter/ip_nat_proto_gre.c
--- linux-2.6.21.orig/net/ipv4/netfilter/ip_nat_proto_gre.c 2007-04-26 
18:10:57.000000000 +0200
+++ linux-2.6.21/net/ipv4/netfilter/ip_nat_proto_gre.c 2007-04-26 
19:01:02.000000000 +0200
@@ -70,6 +70,11 @@
  __be16 *keyptr;
  unsigned int min, i, range_size;

+ /* If there is no master conntrack we are not PPTP,
+    do not change tuples */
+ if (!conntrack->master)
+  return 0;
+
  if (maniptype == IP_NAT_MANIP_SRC)
   keyptr = &tuple->src.u.gre.key;
  else
@@ -122,18 +127,9 @@
  if (maniptype == IP_NAT_MANIP_DST) {
   /* key manipulation is always dest */
   switch (greh->version) {
-  case 0:
-   if (!greh->key) {
-    DEBUGP("can't nat GRE w/o key\n");
-    break;
-   }
-   if (greh->csum) {
-    /* FIXME: Never tested this code... */
-    nf_proto_csum_replace4(gre_csum(greh), *pskb,
-       *(gre_key(greh)),
-       tuple->dst.u.gre.key, 0);
-   }
-   *(gre_key(greh)) = tuple->dst.u.gre.key;
+  case GRE_VERSION_1701:
+   /* FIXME: We do not currently NAT any GREv0 packets */
+   /* try to behave like "ip_nat_proto_unknown" */
    break;
   case GRE_VERSION_PPTP:
    DEBUGP("call_id -> 0x%04x\n",
diff -uNr linux-2.6.21.orig/net/ipv4/netfilter/nf_nat_proto_gre.c 
linux-2.6.21/net/ipv4/netfilter/nf_nat_proto_gre.c
--- linux-2.6.21.orig/net/ipv4/netfilter/nf_nat_proto_gre.c 2007-04-26 
18:11:11.000000000 +0200
+++ linux-2.6.21/net/ipv4/netfilter/nf_nat_proto_gre.c 2007-04-26 
19:01:25.000000000 +0200
@@ -72,6 +72,11 @@
  __be16 *keyptr;
  unsigned int min, i, range_size;

+ /* If there is no master conntrack we are not PPTP,
+    do not change tuples */
+ if (!conntrack->master)
+  return 0;
+
  if (maniptype == IP_NAT_MANIP_SRC)
   keyptr = &tuple->src.u.gre.key;
  else
@@ -122,18 +127,9 @@
  if (maniptype != IP_NAT_MANIP_DST)
   return 1;
  switch (greh->version) {
- case 0:
-  if (!greh->key) {
-   DEBUGP("can't nat GRE w/o key\n");
-   break;
-  }
-  if (greh->csum) {
-   /* FIXME: Never tested this code... */
-   nf_proto_csum_replace4(gre_csum(greh), *pskb,
-            *(gre_key(greh)),
-            tuple->dst.u.gre.key, 0);
-  }
-  *(gre_key(greh)) = tuple->dst.u.gre.key;
+ case GRE_VERSION_1701:
+  /* FIXME: We do not currently NAT any GREv0 packets */
+  /* try to behave like "nf_nat_proto_unknown" */
   break;
  case GRE_VERSION_PPTP:
   DEBUGP("call_id -> 0x%04x\n", ntohs(tuple->dst.u.gre.key));

   -Jorge

===============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5   14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- Sin pistachos no hay Rock & Roll...
- Without wicker a basket cannot be made.
==============================================================

[-- Attachment #2: nat-proto-gre-patch --]
[-- Type: application/octet-stream, Size: 4657 bytes --]

diff -uNr -X /root/diff-no-incluir linux-2.6.21.orig/include/linux/netfilter/nf_conntrack_proto_gre.h linux-2.6.21/include/linux/netfilter/nf_conntrack_proto_gre.h
--- linux-2.6.21.orig/include/linux/netfilter/nf_conntrack_proto_gre.h	2007-04-26 18:10:56.000000000 +0200
+++ linux-2.6.21/include/linux/netfilter/nf_conntrack_proto_gre.h	2007-04-26 18:53:58.000000000 +0200
@@ -87,24 +87,6 @@
 /* delete keymap entries */
 void nf_ct_gre_keymap_destroy(struct nf_conn *ct);
 
-/* get pointer to gre key, if present */
-static inline __be32 *gre_key(struct gre_hdr *greh)
-{
-	if (!greh->key)
-		return NULL;
-	if (greh->csum || greh->routing)
-		return (__be32 *)(greh+sizeof(*greh)+4);
-	return (__be32 *)(greh+sizeof(*greh));
-}
-
-/* get pointer ot gre csum, if present */
-static inline __sum16 *gre_csum(struct gre_hdr *greh)
-{
-	if (!greh->csum)
-		return NULL;
-	return (__sum16 *)(greh+sizeof(*greh));
-}
-
 extern void nf_ct_gre_keymap_flush(void);
 extern void nf_nat_need_gre(void);
 
diff -uNr -X /root/diff-no-incluir linux-2.6.21.orig/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h linux-2.6.21/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h
--- linux-2.6.21.orig/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h	2007-04-26 18:10:56.000000000 +0200
+++ linux-2.6.21/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h	2007-04-26 18:54:39.000000000 +0200
@@ -90,25 +90,6 @@
 /* delete keymap entries */
 void ip_ct_gre_keymap_destroy(struct ip_conntrack *ct);
 
-
-/* get pointer to gre key, if present */
-static inline __be32 *gre_key(struct gre_hdr *greh)
-{
-	if (!greh->key)
-		return NULL;
-	if (greh->csum || greh->routing)
-		return (__be32 *) (greh+sizeof(*greh)+4);
-	return (__be32 *) (greh+sizeof(*greh));
-}
-
-/* get pointer ot gre csum, if present */
-static inline __sum16 *gre_csum(struct gre_hdr *greh)
-{
-	if (!greh->csum)
-		return NULL;
-	return (__sum16 *) (greh+sizeof(*greh));
-}
-
 #endif /* __KERNEL__ */
 
 #endif /* _CONNTRACK_PROTO_GRE_H */
diff -uNr -X /root/diff-no-incluir linux-2.6.21.orig/net/ipv4/netfilter/ip_nat_proto_gre.c linux-2.6.21/net/ipv4/netfilter/ip_nat_proto_gre.c
--- linux-2.6.21.orig/net/ipv4/netfilter/ip_nat_proto_gre.c	2007-04-26 18:10:57.000000000 +0200
+++ linux-2.6.21/net/ipv4/netfilter/ip_nat_proto_gre.c	2007-04-26 19:01:02.000000000 +0200
@@ -70,6 +70,11 @@
 	__be16 *keyptr;
 	unsigned int min, i, range_size;
 
+	/* If there is no master conntrack we are not PPTP,
+	   do not change tuples */
+	if (!conntrack->master)
+		return 0;
+		
 	if (maniptype == IP_NAT_MANIP_SRC)
 		keyptr = &tuple->src.u.gre.key;
 	else
@@ -122,18 +127,9 @@
 	if (maniptype == IP_NAT_MANIP_DST) {
 		/* key manipulation is always dest */
 		switch (greh->version) {
-		case 0:
-			if (!greh->key) {
-				DEBUGP("can't nat GRE w/o key\n");
-				break;
-			}
-			if (greh->csum) {
-				/* FIXME: Never tested this code... */
-				nf_proto_csum_replace4(gre_csum(greh), *pskb,
-							*(gre_key(greh)),
-							tuple->dst.u.gre.key, 0);
-			}
-			*(gre_key(greh)) = tuple->dst.u.gre.key;
+		case GRE_VERSION_1701:
+			/* FIXME: We do not currently NAT any GREv0 packets */
+			/* try to behave like "ip_nat_proto_unknown" */
 			break;
 		case GRE_VERSION_PPTP:
 			DEBUGP("call_id -> 0x%04x\n",
diff -uNr -X /root/diff-no-incluir linux-2.6.21.orig/net/ipv4/netfilter/nf_nat_proto_gre.c linux-2.6.21/net/ipv4/netfilter/nf_nat_proto_gre.c
--- linux-2.6.21.orig/net/ipv4/netfilter/nf_nat_proto_gre.c	2007-04-26 18:11:11.000000000 +0200
+++ linux-2.6.21/net/ipv4/netfilter/nf_nat_proto_gre.c	2007-04-26 19:01:25.000000000 +0200
@@ -72,6 +72,11 @@
 	__be16 *keyptr;
 	unsigned int min, i, range_size;
 
+	/* If there is no master conntrack we are not PPTP,
+	   do not change tuples */
+	if (!conntrack->master)
+		return 0;
+		
 	if (maniptype == IP_NAT_MANIP_SRC)
 		keyptr = &tuple->src.u.gre.key;
 	else
@@ -122,18 +127,9 @@
 	if (maniptype != IP_NAT_MANIP_DST)
 		return 1;
 	switch (greh->version) {
-	case 0:
-		if (!greh->key) {
-			DEBUGP("can't nat GRE w/o key\n");
-			break;
-		}
-		if (greh->csum) {
-			/* FIXME: Never tested this code... */
-			nf_proto_csum_replace4(gre_csum(greh), *pskb,
-					       *(gre_key(greh)),
-					       tuple->dst.u.gre.key, 0);
-		}
-		*(gre_key(greh)) = tuple->dst.u.gre.key;
+	case GRE_VERSION_1701:
+		/* FIXME: We do not currently NAT any GREv0 packets */
+		/* try to behave like "nf_nat_proto_unknown" */
 		break;
 	case GRE_VERSION_PPTP:
 		DEBUGP("call_id -> 0x%04x\n", ntohs(tuple->dst.u.gre.key));

Re: [PATCH] xx_nat_proto_gre: gre_key returns wrong pointer

[Patrick McHardy]

Jorge Boncompte [DTI2] wrote:
>    Hi, Patrick take a look at this patch when you have time please. I
> tested it and behaves correctly, I can run one and only one GRE tunnel
> though a NATing box and PPTP still works too, if you try with a second
> tunnel from different host by the fact of not modifying the tuple it does
> not work as the comment in get_unique_tuple says.

> +++ linux-2.6.21/net/ipv4/netfilter/ip_nat_proto_gre.c 2007-04-26
> 19:01:02.000000000 +0200
> @@ -70,6 +70,11 @@
>  __be16 *keyptr;
>  unsigned int min, i, range_size;
> 
> + /* If there is no master conntrack we are not PPTP,
> +    do not change tuples */
> + if (!conntrack->master)
> +  return 0;
> +


Thats a good idea, I thought it would be more complicated to
determine the kind of connection in get_unique_tuple() since
it doesn't get to see the packet.

Please sign off on the patch so I can apply it.

[PATCH] xx_nat_proto_gre: do not modify/corrupt GREv0 packets thought NAT

[Jorge Boncompte [DTI2]]

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

----- Original Message ----- 
From: "Patrick McHardy" <kaber@trash.net>
To: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Cc: <netfilter-devel@lists.netfilter.org>
Sent: Friday, April 27, 2007 1:21 PM
Subject: Re: [PATCH] xx_nat_proto_gre: gre_key returns wrong pointer


>> + /* If there is no master conntrack we are not PPTP,
>> +    do not change tuples */
>> + if (!conntrack->master)
>> +  return 0;
>> +
>
>
> Thats a good idea, I thought it would be more complicated to
> determine the kind of connection in get_unique_tuple() since
> it doesn't get to see the packet.
>
> Please sign off on the patch so I can apply it.

    To be fair I thought that it were to be more complicated too and the 
only way that I could see to determine the kind of connection was to add the 
version field again to the tuple. What ringed some bells in my head was that 
I saw a line "no master !?!" from the DEBUGP statement on 
xx_conntrack_proto_gre::gre_destroy while looking at the logs ;-)

    -Jorge

---
From: Jorge Boncompte <jorge@dti2.net>

    Hi,

    While porting some changes of the 2.6.21-rc7 pptp/proto_gre conntrack
and nat modules to a 2.4.32 kernel I noticed that the gre_key function
returns a wrong pointer to the GRE key of a version 0 packet thus corrupting
the packet payload.
    The intended behaviour for GREv0 packets is to act like
nf_conntrack_proto_generic/nf_nat_proto_unknown so I have ripped the
offending functions (not used anymore) and modified the xx_nat_proto_gre
modules to not touch version 0 (non PPTP) packets.

Signed-off-by: Jorge Boncompte <jorge@dti2.net>

---
diff -uNr linux-2.6.21.orig/include/linux/netfilter/nf_conntrack_proto_gre.h 
linux-2.6.21/include/linux/netfilter/nf_conntrack_proto_gre.h
--- linux-2.6.21.orig/include/linux/netfilter/nf_conntrack_proto_gre.h 
2007-04-26 18:10:56.000000000 +0200
+++ linux-2.6.21/include/linux/netfilter/nf_conntrack_proto_gre.h 2007-04-26 
18:53:58.000000000 +0200
@@ -87,24 +87,6 @@
 /* delete keymap entries */
 void nf_ct_gre_keymap_destroy(struct nf_conn *ct);

-/* get pointer to gre key, if present */
-static inline __be32 *gre_key(struct gre_hdr *greh)
-{
- if (!greh->key)
-  return NULL;
- if (greh->csum || greh->routing)
-  return (__be32 *)(greh+sizeof(*greh)+4);
- return (__be32 *)(greh+sizeof(*greh));
-}
-
-/* get pointer ot gre csum, if present */
-static inline __sum16 *gre_csum(struct gre_hdr *greh)
-{
- if (!greh->csum)
-  return NULL;
- return (__sum16 *)(greh+sizeof(*greh));
-}
-
 extern void nf_ct_gre_keymap_flush(void);
 extern void nf_nat_need_gre(void);

diff -uNr 
linux-2.6.21.orig/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h 
linux-2.6.21/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h
--- linux-2.6.21.orig/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h 
2007-04-26 18:10:56.000000000 +0200
+++ linux-2.6.21/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h 
2007-04-26 18:54:39.000000000 +0200
@@ -90,25 +90,6 @@
 /* delete keymap entries */
 void ip_ct_gre_keymap_destroy(struct ip_conntrack *ct);

-
-/* get pointer to gre key, if present */
-static inline __be32 *gre_key(struct gre_hdr *greh)
-{
- if (!greh->key)
-  return NULL;
- if (greh->csum || greh->routing)
-  return (__be32 *) (greh+sizeof(*greh)+4);
- return (__be32 *) (greh+sizeof(*greh));
-}
-
-/* get pointer ot gre csum, if present */
-static inline __sum16 *gre_csum(struct gre_hdr *greh)
-{
- if (!greh->csum)
-  return NULL;
- return (__sum16 *) (greh+sizeof(*greh));
-}
-
 #endif /* __KERNEL__ */

 #endif /* _CONNTRACK_PROTO_GRE_H */
diff -uNr -X /root/diff-no-incluir 
linux-2.6.21.orig/net/ipv4/netfilter/ip_nat_proto_gre.c 
linux-2.6.21/net/ipv4/netfilter/ip_nat_proto_gre.c
--- linux-2.6.21.orig/net/ipv4/netfilter/ip_nat_proto_gre.c 2007-04-26 
18:10:57.000000000 +0200
+++ linux-2.6.21/net/ipv4/netfilter/ip_nat_proto_gre.c 2007-04-26 
19:01:02.000000000 +0200
@@ -70,6 +70,11 @@
  __be16 *keyptr;
  unsigned int min, i, range_size;

+ /* If there is no master conntrack we are not PPTP,
+    do not change tuples */
+ if (!conntrack->master)
+  return 0;
+
  if (maniptype == IP_NAT_MANIP_SRC)
   keyptr = &tuple->src.u.gre.key;
  else
@@ -122,18 +127,9 @@
  if (maniptype == IP_NAT_MANIP_DST) {
   /* key manipulation is always dest */
   switch (greh->version) {
-  case 0:
-   if (!greh->key) {
-    DEBUGP("can't nat GRE w/o key\n");
-    break;
-   }
-   if (greh->csum) {
-    /* FIXME: Never tested this code... */
-    nf_proto_csum_replace4(gre_csum(greh), *pskb,
-       *(gre_key(greh)),
-       tuple->dst.u.gre.key, 0);
-   }
-   *(gre_key(greh)) = tuple->dst.u.gre.key;
+  case GRE_VERSION_1701:
+   /* FIXME: We do not currently NAT any GREv0 packets */
+   /* try to behave like "ip_nat_proto_unknown" */
    break;
   case GRE_VERSION_PPTP:
    DEBUGP("call_id -> 0x%04x\n",
diff -uNr linux-2.6.21.orig/net/ipv4/netfilter/nf_nat_proto_gre.c 
linux-2.6.21/net/ipv4/netfilter/nf_nat_proto_gre.c
--- linux-2.6.21.orig/net/ipv4/netfilter/nf_nat_proto_gre.c 2007-04-26 
18:11:11.000000000 +0200
+++ linux-2.6.21/net/ipv4/netfilter/nf_nat_proto_gre.c 2007-04-26 
19:01:25.000000000 +0200
@@ -72,6 +72,11 @@
  __be16 *keyptr;
  unsigned int min, i, range_size;

+ /* If there is no master conntrack we are not PPTP,
+    do not change tuples */
+ if (!conntrack->master)
+  return 0;
+
  if (maniptype == IP_NAT_MANIP_SRC)
   keyptr = &tuple->src.u.gre.key;
  else
@@ -122,18 +127,9 @@
  if (maniptype != IP_NAT_MANIP_DST)
   return 1;
  switch (greh->version) {
- case 0:
-  if (!greh->key) {
-   DEBUGP("can't nat GRE w/o key\n");
-   break;
-  }
-  if (greh->csum) {
-   /* FIXME: Never tested this code... */
-   nf_proto_csum_replace4(gre_csum(greh), *pskb,
-            *(gre_key(greh)),
-            tuple->dst.u.gre.key, 0);
-  }
-  *(gre_key(greh)) = tuple->dst.u.gre.key;
+ case GRE_VERSION_1701:
+  /* FIXME: We do not currently NAT any GREv0 packets */
+  /* try to behave like "nf_nat_proto_unknown" */
   break;
  case GRE_VERSION_PPTP:
   DEBUGP("call_id -> 0x%04x\n", ntohs(tuple->dst.u.gre.key));



[-- Attachment #2: nat-proto-gre-patch --]
[-- Type: application/octet-stream, Size: 4557 bytes --]

diff -uNr linux-2.6.21.orig/include/linux/netfilter/nf_conntrack_proto_gre.h linux-2.6.21/include/linux/netfilter/nf_conntrack_proto_gre.h
--- linux-2.6.21.orig/include/linux/netfilter/nf_conntrack_proto_gre.h	2007-04-26 18:10:56.000000000 +0200
+++ linux-2.6.21/include/linux/netfilter/nf_conntrack_proto_gre.h	2007-04-26 18:53:58.000000000 +0200
@@ -87,24 +87,6 @@
 /* delete keymap entries */
 void nf_ct_gre_keymap_destroy(struct nf_conn *ct);
 
-/* get pointer to gre key, if present */
-static inline __be32 *gre_key(struct gre_hdr *greh)
-{
-	if (!greh->key)
-		return NULL;
-	if (greh->csum || greh->routing)
-		return (__be32 *)(greh+sizeof(*greh)+4);
-	return (__be32 *)(greh+sizeof(*greh));
-}
-
-/* get pointer ot gre csum, if present */
-static inline __sum16 *gre_csum(struct gre_hdr *greh)
-{
-	if (!greh->csum)
-		return NULL;
-	return (__sum16 *)(greh+sizeof(*greh));
-}
-
 extern void nf_ct_gre_keymap_flush(void);
 extern void nf_nat_need_gre(void);
 
diff -uNr linux-2.6.21.orig/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h linux-2.6.21/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h
--- linux-2.6.21.orig/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h	2007-04-26 18:10:56.000000000 +0200
+++ linux-2.6.21/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h	2007-04-26 18:54:39.000000000 +0200
@@ -90,25 +90,6 @@
 /* delete keymap entries */
 void ip_ct_gre_keymap_destroy(struct ip_conntrack *ct);
 
-
-/* get pointer to gre key, if present */
-static inline __be32 *gre_key(struct gre_hdr *greh)
-{
-	if (!greh->key)
-		return NULL;
-	if (greh->csum || greh->routing)
-		return (__be32 *) (greh+sizeof(*greh)+4);
-	return (__be32 *) (greh+sizeof(*greh));
-}
-
-/* get pointer ot gre csum, if present */
-static inline __sum16 *gre_csum(struct gre_hdr *greh)
-{
-	if (!greh->csum)
-		return NULL;
-	return (__sum16 *) (greh+sizeof(*greh));
-}
-
 #endif /* __KERNEL__ */
 
 #endif /* _CONNTRACK_PROTO_GRE_H */
diff -uNr linux-2.6.21.orig/net/ipv4/netfilter/ip_nat_proto_gre.c linux-2.6.21/net/ipv4/netfilter/ip_nat_proto_gre.c
--- linux-2.6.21.orig/net/ipv4/netfilter/ip_nat_proto_gre.c	2007-04-26 18:10:57.000000000 +0200
+++ linux-2.6.21/net/ipv4/netfilter/ip_nat_proto_gre.c	2007-04-26 19:01:02.000000000 +0200
@@ -70,6 +70,11 @@
 	__be16 *keyptr;
 	unsigned int min, i, range_size;
 
+	/* If there is no master conntrack we are not PPTP,
+	   do not change tuples */
+	if (!conntrack->master)
+		return 0;
+		
 	if (maniptype == IP_NAT_MANIP_SRC)
 		keyptr = &tuple->src.u.gre.key;
 	else
@@ -122,18 +127,9 @@
 	if (maniptype == IP_NAT_MANIP_DST) {
 		/* key manipulation is always dest */
 		switch (greh->version) {
-		case 0:
-			if (!greh->key) {
-				DEBUGP("can't nat GRE w/o key\n");
-				break;
-			}
-			if (greh->csum) {
-				/* FIXME: Never tested this code... */
-				nf_proto_csum_replace4(gre_csum(greh), *pskb,
-							*(gre_key(greh)),
-							tuple->dst.u.gre.key, 0);
-			}
-			*(gre_key(greh)) = tuple->dst.u.gre.key;
+		case GRE_VERSION_1701:
+			/* FIXME: We do not currently NAT any GREv0 packets */
+			/* try to behave like "ip_nat_proto_unknown" */
 			break;
 		case GRE_VERSION_PPTP:
 			DEBUGP("call_id -> 0x%04x\n",
diff -uNr linux-2.6.21.orig/net/ipv4/netfilter/nf_nat_proto_gre.c linux-2.6.21/net/ipv4/netfilter/nf_nat_proto_gre.c
--- linux-2.6.21.orig/net/ipv4/netfilter/nf_nat_proto_gre.c	2007-04-26 18:11:11.000000000 +0200
+++ linux-2.6.21/net/ipv4/netfilter/nf_nat_proto_gre.c	2007-04-26 19:01:25.000000000 +0200
@@ -72,6 +72,11 @@
 	__be16 *keyptr;
 	unsigned int min, i, range_size;
 
+	/* If there is no master conntrack we are not PPTP,
+	   do not change tuples */
+	if (!conntrack->master)
+		return 0;
+		
 	if (maniptype == IP_NAT_MANIP_SRC)
 		keyptr = &tuple->src.u.gre.key;
 	else
@@ -122,18 +127,9 @@
 	if (maniptype != IP_NAT_MANIP_DST)
 		return 1;
 	switch (greh->version) {
-	case 0:
-		if (!greh->key) {
-			DEBUGP("can't nat GRE w/o key\n");
-			break;
-		}
-		if (greh->csum) {
-			/* FIXME: Never tested this code... */
-			nf_proto_csum_replace4(gre_csum(greh), *pskb,
-					       *(gre_key(greh)),
-					       tuple->dst.u.gre.key, 0);
-		}
-		*(gre_key(greh)) = tuple->dst.u.gre.key;
+	case GRE_VERSION_1701:
+		/* FIXME: We do not currently NAT any GREv0 packets */
+		/* try to behave like "nf_nat_proto_unknown" */
 		break;
 	case GRE_VERSION_PPTP:
 		DEBUGP("call_id -> 0x%04x\n", ntohs(tuple->dst.u.gre.key));

Re: [PATCH] xx_nat_proto_gre: do not modify/corrupt GREv0 packets thought NAT

[Patrick McHardy]

Jorge Boncompte [DTI2] wrote:
>    While porting some changes of the 2.6.21-rc7 pptp/proto_gre conntrack
> and nat modules to a 2.4.32 kernel I noticed that the gre_key function
> returns a wrong pointer to the GRE key of a version 0 packet thus
> corrupting
> the packet payload.
>    The intended behaviour for GREv0 packets is to act like
> nf_conntrack_proto_generic/nf_nat_proto_unknown so I have ripped the
> offending functions (not used anymore) and modified the xx_nat_proto_gre
> modules to not touch version 0 (non PPTP) packets.


Applied, thanks. I removed the FIXME though since its the intended
behaviour and not something that needs to be fixed. I'll push it
to -stable as well.

Re: [PATCH] xx_nat_proto_gre: do not modify/corrupt GREv0 packets thought NAT

[Jorge Boncompte [DTI2]]

----- Original Message ----- 
From: "Patrick McHardy" <kaber@trash.net>
To: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Cc: <netfilter-devel@lists.netfilter.org>
Sent: Wednesday, May 02, 2007 2:25 PM
Subject: Re: [PATCH] xx_nat_proto_gre: do not modify/corrupt GREv0 packets 
thought NAT


> Applied, thanks. I removed the FIXME though since its the intended
> behaviour and not something that needs to be fixed. I'll push it
> to -stable as well.

    Well, I don't have an opinion on the comment. My only intention was to 
reflect the fact that we do not NAT those packets as the comment states.

    Just for the records:
    The code can be made to NAT GREv0 packets with a key, at least if the 
orig and repl direction use the same key. This is the normal behaviour when 
you configure GRE tunnels on Cisco gears, Linux "ip tunnel" allows to use 
different keys for transmitting and receiving. I have tested that SNAT 
tracks the packets and that I can use several tunnels between the same 
endpoints with different keys, it did require only some minor modifications 
but to do it right it will need some more changes like to expand the key 
field to a 32bit type again all over the code.
    If someone ever needs it, just ask.

    Best regards,

    -Jorge

==============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5   14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- Sin pistachos no hay Rock & Roll...
- Without wicker a basket cannot be made.
==============================================================

Re: [PATCH] xx_nat_proto_gre: do not modify/corrupt GREv0 packets thought NAT

[Patrick McHardy]

Jorge Boncompte [DTI2] wrote:
> ----- Original Message ----- From: "Patrick McHardy" <kaber@trash.net>
>> Applied, thanks. I removed the FIXME though since its the intended
>> behaviour and not something that needs to be fixed. I'll push it
>> to -stable as well.
> 
> 
>    Well, I don't have an opinion on the comment. My only intention was
> to reflect the fact that we do not NAT those packets as the comment states.


Yes, I left that part intact, I just removed the FIXME.

>    Just for the records:
>    The code can be made to NAT GREv0 packets with a key, at least if the
> orig and repl direction use the same key. This is the normal behaviour
> when you configure GRE tunnels on Cisco gears, Linux "ip tunnel" allows
> to use different keys for transmitting and receiving. I have tested that
> SNAT tracks the packets and that I can use several tunnels between the
> same endpoints with different keys, it did require only some minor
> modifications but to do it right it will need some more changes like to
> expand the key field to a 32bit type again all over the code.
>    If someone ever needs it, just ask.


I think the problem with this is that we don't know whether both keys
are identical at connection setup time and thus might fail to even
track the connection if they are not.

If thats not correct feel free to send a patch on top of the previous
one :)

Re: [PATCH] xx_nat_proto_gre: do not modify/corrupt GREv0 packets thought NAT

[Jorge Boncompte [DTI2]]

----- Original Message ----- 
From: "Patrick McHardy" <kaber@trash.net>
To: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Cc: <netfilter-devel@lists.netfilter.org>
Sent: Wednesday, May 02, 2007 3:23 PM
Subject: Re: [PATCH] xx_nat_proto_gre: do not modify/corrupt GREv0 packets 
thought NAT


>>    The code can be made to NAT GREv0 packets with a key, at least if the
>> orig and repl direction use the same key. This is the normal behaviour
>> when you configure GRE tunnels on Cisco gears, Linux "ip tunnel" allows
>> to use different keys for transmitting and receiving. I have tested that
>> SNAT tracks the packets and that I can use several tunnels between the
>> same endpoints with different keys, it did require only some minor
>> modifications but to do it right it will need some more changes like to
>> expand the key field to a 32bit type again all over the code.
>>    If someone ever needs it, just ask.
>
>
> I think the problem with this is that we don't know whether both keys
> are identical at connection setup time and thus might fail to even
> track the connection if they are not.
>
> If thats not correct feel free to send a patch on top of the previous
> one :)

    Yes, you are right, we don't know if both keys are identical as there is 
nothing like a "key exchange" before. So we will only support, as I stated, 
the connections that have the same key. And I even did not try to DNAT the 
packets.
    I have not thinked much about it but for a "full"(only connections with 
same key) solution we would need something alongside the xt_tcpudp.c (and 
userspace code) where we could match different keys to allow the DNAT code 
to redirect the connections to different hosts.
    The SNAT part only should be easy but I don't know if that is likely to 
be accepted. What's your opinion?

    -Jorge

==============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5   14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- Sin pistachos no hay Rock & Roll...
- Without wicker a basket cannot be made.
==============================================================

Re: [PATCH] xx_nat_proto_gre: do not modify/corrupt GREv0 packets thought NAT

[Patrick McHardy]

Jorge Boncompte [DTI2] wrote:
> ----- Original Message ----- From: "Patrick McHardy" <kaber@trash.net>
>> I think the problem with this is that we don't know whether both keys
>> are identical at connection setup time and thus might fail to even
>> track the connection if they are not.
> 
> 
>    Yes, you are right, we don't know if both keys are identical as there
> is nothing like a "key exchange" before. So we will only support, as I
> stated, the connections that have the same key. And I even did not try
> to DNAT the packets.


The problem is that this at the same time causes us *not* to work
properly anymore with connections with different keys, right?

>    I have not thinked much about it but for a "full"(only connections
> with same key) solution we would need something alongside the
> xt_tcpudp.c (and userspace code) where we could match different keys to
> allow the DNAT code to redirect the connections to different hosts.
>    The SNAT part only should be easy but I don't know if that is likely
> to be accepted. What's your opinion?


I'll take it a patch if doesn't break something else (like connections
with different keys).