[PATCH][2.5.22] OOPS in tcp_v6_get_port

[PATCH][2.5.22] OOPS in tcp_v6_get_port

[Carl Ritson]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2454 bytes --]


Sorry for the repeat email but this bug is also in 2.5.22 and my patch is still
valid, although I'm not entirely sure it is the correct fix for the problem?

-- From Previous email --
2.5.21 and 2.5.22 OOPS at boot on my test machine, the decoded OOPS is attached.
Also attached is a program that triggers the bug, it emulates the behavior 
of bind on my test machine and binds to two ports one IPv4 and one IPv6 
with the same port number but different IP addresses.

The bug appears to be the IPv6 TCP code, in net/ipv6/tcp_ipv6.c
Line 149:
	struct ipv6_pinfo *np2 = inet6_sk(sk2);

	if (sk != sk2 &&
	    sk->bound_dev_if == sk2->bound_dev_if) {
		if (!sk_reuse	||
		    !sk2->reuse	||
		    sk2->state == TCP_LISTEN) {
			/* NOTE: IPv6 tw bucket have different format */
			if (!inet_sk(sk2)->rcv_saddr ||
			    addr_type == IPV6_ADDR_ANY ||
			    !ipv6_addr_cmp(&np->rcv_saddr,
			    		sk2->state != TCP_TIME_WAIT ?
BUG -->					&np2->rcv_saddr :
					&((struct tcp_tw_bucket*)sk)->v6_rcv_saddr) ||
			    (addr_type==IPV6_ADDR_MAPPED && sk2->family==AF_INET &&
			     inet_sk(sk)->rcv_saddr ==
			     inet_sk(sk2)->rcv_saddr))
				break;
			}
		}
	}

np2 can be NULL if the socket is an IPv4 socket, since IPv6 and IPv4 
share the port address space.  While the test of !inet_sk(sk2-)->rcv_addr 
_should_ prevent this it is not assured in C that the conditions to an if 
statement will be evaluated in the order written?  At least my gcc 
(2.95.4 from compiled from CVS) doesn't think so :-).

I propose a diff similar to the one below to fix the problem maybe?

Many thanks,
Carl Ritson
critson@perlfu.co.uk

--- orig/net/ipv6/tcp_ipv6.c	Sat Jun 15 19:23:44 2002
+++ linux/net/ipv6/tcp_ipv6.c	Sat Jun 15 19:21:46 2002
@@ -156,14 +156,16 @@
 						/* NOTE: IPv6 tw bucket have different format */
 						if (!inet_sk(sk2)->rcv_saddr ||
 						    addr_type == IPV6_ADDR_ANY ||
-						    !ipv6_addr_cmp(&np->rcv_saddr,
-								   sk2->state != TCP_TIME_WAIT ?
-								   &np2->rcv_saddr :
-								   &((struct tcp_tw_bucket*)sk)->v6_rcv_saddr) ||
 						    (addr_type==IPV6_ADDR_MAPPED && sk2->family==AF_INET &&
 						     inet_sk(sk)->rcv_saddr ==
 						     inet_sk(sk2)->rcv_saddr))
 							break;
+						if (np2 != NULL) 
+							if (!ipv6_addr_cmp(&np->rcv_saddr,
+								   sk2->state != TCP_TIME_WAIT ?
+								   &np2->rcv_saddr :
+								   &((struct tcp_tw_bucket*)sk)->v6_rcv_saddr))
+								break;
 					}
 				}
 			}


[-- Attachment #2: Type: TEXT/PLAIN, Size: 2217 bytes --]

ksymoops 2.4.4 on i686 2.4.19-pre8-ac2.  Options used
     -v /usr/src/linux-2.5.21/vmlinux (specified)
     -K (specified)
     -L (specified)
     -o /lib/modules/2.5.21/ (specified)
     -m /boot/System.map-2.5.21 (specified)

No modules in ksyms, skipping objects
Unable to handle kernel NULL pointer dereference at virtual address 00000010
c027244e
*pde = 00000000
Oops: 0000
CPU:    0
EIP:    0010:[<c027244e>]  Not tainted
Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00010246
eax: cfd68b28   ebx: cea60360     ecx: 00000010       edx: cfd68a0a
esi: cfd68de8   edi: 00000010     ebp: cf9cf260       esp: ce8a7e98
ds: 0018   es: 0018   ss: 0018
Stack:  ce8a7f1c cfd68ac0 cfd68de8 c0341908 cfd68de8 0100007f 00000011 00000001
        cfd68ac0 cfd21dc8 03b93874 c0260f81 cfd68ac0 000003b9 ce8a7a40 ce8a7f14
        0000001c 40263874 00000011 03b919c8 0600007f cfd68dd8 cfd68bf8 c02227c0
Call Trace: [<c0260f81>] [<c02227c0>] [c0223629>] [<c0221cf5>] [<c0222d2e>]
   [<c0222d50>] [<c02232a0>] [<c0106a17>]
Code: f3 a6 74 2c 81 7c 24 18 00 10 00 00 75 17 66 83 7b 1c 02 75

>>EIP; c027244e <tcp_v6_get_port+1ae/2d4>   <=====
Trace; c0260f81 <inet6_bind+2b1/3d8>
Trace; c02227c0 <sys_bind+54/74>
Trace; c0222d50 <sys_setsockopt+6c/78>
Trace; c02232a0 <sys_socketcall+74/1fc>
Trace; c0106a17 <syscall_call+7/b>
Code;  c027244e <tcp_v6_get_port+1ae/2d4>
00000000 <_EIP>:
Code;  c027244e <tcp_v6_get_port+1ae/2d4>   <=====
   0:   f3 a6                     repz cmpsb %es:(%edi),%ds:(%esi)   <=====
Code;  c0272450 <tcp_v6_get_port+1b0/2d4>
   2:   74 2c                     je     30 <_EIP+0x30> c027247e <tcp_v6_get_port+1de/2d4>
Code;  c0272452 <tcp_v6_get_port+1b2/2d4>
   4:   81 7c 24 18 00 10 00      cmpl   $0x1000,0x18(%esp,1)
Code;  c0272459 <tcp_v6_get_port+1b9/2d4>
   b:   00 
Code;  c027245a <tcp_v6_get_port+1ba/2d4>
   c:   75 17                     jne    25 <_EIP+0x25> c0272473 <tcp_v6_get_port+1d3/2d4>
Code;  c027245c <tcp_v6_get_port+1bc/2d4>
   e:   66 83 7b 1c 02            cmpw   $0x2,0x1c(%ebx)
Code;  c0272461 <tcp_v6_get_port+1c1/2d4>
  13:   75 00                     jne    15 <_EIP+0x15> c0272463 <tcp_v6_get_port+1c3/2d4>


[-- Attachment #3: Type: TEXT/PLAIN, Size: 1593 bytes --]

#include <stdlib.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>

const unsigned short bind_port = 4500;

int do_ipv4_bind(char *ip, const unsigned short port) {
	struct sockaddr_in addr;
	int sock;
	int ret;
	
	memset(&addr, 0x00, sizeof(addr));
	addr.sin_family = AF_INET;
	addr.sin_port = htons(port);
	inet_pton(AF_INET, ip, &(addr.sin_addr));
	
	ret = socket(PF_INET, SOCK_STREAM, 0);
	if(ret == -1) {
		return ret;
	} else {
		sock = ret;
	}
	ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));
	if(ret == -1) {
		return ret;
	} else {
		return sock;
	}
}

int do_ipv6_bind(char *ip, const unsigned short port) {
	struct sockaddr_in6 addr;
	int sock;
	int ret;
	
	memset(&addr, 0x00, sizeof(addr));
	addr.sin6_family = AF_INET6;
	addr.sin6_port = htons(port);
	inet_pton(AF_INET6, ip, &(addr.sin6_addr));
	
	ret = socket(PF_INET6, SOCK_STREAM, 0);
	if(ret == -1) {
		return ret;
	} else {
		sock = ret;
	}

	ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));
	if(ret == -1) {
		return ret;
	} else {
		return sock;
	}
}

int main(void) {
	int ipv4_sock = 0;
	int ipv6_sock = 0;
	ipv4_sock = do_ipv4_bind("127.0.0.1", bind_port);
	ipv6_sock = do_ipv6_bind("::1", bind_port);
	if(ipv4_sock < 0) {
		printf("ipv4 bind failed\n");
	} else {
		printf("ipv4 bind succeeded\n");
		close(ipv4_sock);
	}
	if(ipv6_sock < 0) {
		printf("ipv6 bind failed\n");
	} else {
		printf("ipv6 bind succeeded\n");
		close(ipv6_sock);
	}
	return 0;
}

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[David S. Miller]

This is a known bug introduced by the struct sock splitup into
external per-protocol pieces done by Arnaldo de Melo.  He is working
on the proper fix, your proposed change will just paper over the real
bug.

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Arnaldo Carvalho de Melo]

Em Mon, Jun 17, 2002 at 02:33:19PM -0700, David S. Miller escreveu:
> 
> This is a known bug introduced by the struct sock splitup into
> external per-protocol pieces done by Arnaldo de Melo.  He is working
> on the proper fix, your proposed change will just paper over the real
> bug.

Carl,

	Can you try this patch?

- Arnaldo

--- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
+++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002
@@ -1240,6 +1240,7 @@
 					  struct dst_entry *dst)
 {
 	struct ipv6_pinfo *newnp, *np = inet6_sk(sk);
+	struct tcp6_sock *newtcp6sk;
 	struct flowi fl;
 	struct inet_opt *newinet;
 	struct tcp_opt *newtp;
@@ -1256,10 +1257,15 @@
 		if (newsk == NULL) 
 			return NULL;
 
+		newtcp6sk = (struct tcp6_sock *)newsk;
+		newtcp6sk->pinet6 = &newtcp6sk->inet6;
+
 		newinet = inet_sk(newsk);
 		newnp = inet6_sk(newsk);
 		newtp = tcp_sk(newsk);
 
+		memcpy(newnp, np, sizeof(struct ipv6_pinfo));
+
 		ipv6_addr_set(&newnp->daddr, 0, 0, htonl(0x0000FFFF),
 			      newinet->daddr);
 
@@ -1336,9 +1342,15 @@
 	ip6_dst_store(newsk, dst, NULL);
 	sk->route_caps = dst->dev->features&~NETIF_F_IP_CSUM;
 
+	newtcp6sk = (struct tcp6_sock *)newsk;
+	newtcp6sk->pinet6 = &newtcp6sk->inet6;
+
 	newtp = tcp_sk(newsk);
 	newinet = inet_sk(newsk);
 	newnp = inet6_sk(newsk);
+
+	memcpy(newnp, np, sizeof(struct ipv6_pinfo));
+
 	ipv6_addr_copy(&newnp->daddr, &req->af.v6_req.rmt_addr);
 	ipv6_addr_copy(&newnp->saddr, &req->af.v6_req.loc_addr);
 	ipv6_addr_copy(&newnp->rcv_saddr, &req->af.v6_req.loc_addr);


-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[David S. Miller]

   From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
   Date: Mon, 17 Jun 2002 21:57:35 -0300

   --- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
   +++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002

I've installed this change into my tree in the meantime.
If we find a better fix, we can just revert this.

Thanks.

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Arnaldo Carvalho de Melo]

Em Mon, Jun 17, 2002 at 07:17:26PM -0700, David S. Miller escreveu:
>    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
>    Date: Mon, 17 Jun 2002 21:57:35 -0300
> 
>    --- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
>    +++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002
> 
> I've installed this change into my tree in the meantime.
> If we find a better fix, we can just revert this.

OK, I found a better fix, I think, that allowed me to kill inet6_sk_generic
in af_inet6.c, using a constructor for the tcpv6, udpv6 and raw6 slab caches,
as suggested by Russel King, I'll be sending RSN.

- Arnaldo

[BKPATCH] Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Arnaldo Carvalho de Melo]

Em Mon, Jun 17, 2002 at 11:49:34PM -0300, Arnaldo C. Melo escreveu:
> Em Mon, Jun 17, 2002 at 07:17:26PM -0700, David S. Miller escreveu:
> >    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
> >    Date: Mon, 17 Jun 2002 21:57:35 -0300
> > 
> >    --- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
> >    +++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002
> > 
> > I've installed this change into my tree in the meantime.
> > If we find a better fix, we can just revert this.
> 
> OK, I found a better fix, I think, that allowed me to kill inet6_sk_generic
> in af_inet6.c, using a constructor for the tcpv6, udpv6 and raw6 slab caches,
> as suggested by Russel King, I'll be sending RSN.

Here it is, David, please consider pulling it from:

http://kernel-acme.bkbits.net:8080/tcpv6-pinet6

Best Regards,

- Arnaldo


# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.656   -> 1.657  
#	 net/ipv6/tcp_ipv6.c	1.17    -> 1.18   
#	 net/ipv6/af_inet6.c	1.8     -> 1.9    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/06/18	acme@conectiva.com.br	1.657
# net/ipv6/af_inet6.c
# net/ipv6/tcp_ipv6.c
# 
# 	- Add constructors for the tcp6_sk_cachep, udp6_sk_cachep and raw6_sk_cachep slab caches,
#           to initialize ->pinet6 to the respective &->inet6, this simplifies inet6_create,
#           making ipv6_sk_generic not needed and fixing a bug where we create a new tcpv6 socket
#           outside inet6_create and we were not initializing ->pinet6 properly, thanks to
#           Russell King for pointing out the bug and doing the initial analisys that made the
#           fix easy to make.
# --------------------------------------------
#
diff -Nru a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
--- a/net/ipv6/af_inet6.c	Mon Mar 11 11:13:05 2002
+++ b/net/ipv6/af_inet6.c	Tue Jun 18 00:27:30 2002
@@ -134,23 +134,11 @@
         return rc;
 }
 
-static __inline__ struct ipv6_pinfo *inet6_sk_generic(struct sock *sk)
-{
-	struct ipv6_pinfo *rc = (&((struct tcp6_sock *)sk)->inet6);
-
-        if (sk->protocol == IPPROTO_UDP)
-                rc = (&((struct udp6_sock *)sk)->inet6);
-        else if (sk->protocol == IPPROTO_RAW)
-                rc = (&((struct raw6_sock *)sk)->inet6);
-        return rc;
-}
-
 static int inet6_create(struct socket *sock, int protocol)
 {
 	struct inet_opt *inet;
 	struct ipv6_pinfo *np;
 	struct sock *sk;
-	struct tcp6_sock* tcp6sk;
 	struct list_head *p;
 	struct inet_protosw *answer;
 
@@ -212,8 +200,7 @@
 
 	sk->backlog_rcv		= answer->prot->backlog_rcv;
 
-	tcp6sk		= (struct tcp6_sock *)sk;
-	tcp6sk->pinet6 = np = inet6_sk_generic(sk);
+	np		= inet6_sk(sk);
 	np->hop_limit	= -1;
 	np->mcast_hops	= -1;
 	np->mc_loop	= 1;
@@ -632,6 +619,30 @@
 	inet_unregister_protosw(p);
 }
 
+static void tcp6_pinet6_init(void *foo, kmem_cache_t *cachep,
+			     unsigned long flags)
+{
+	struct tcp6_sock *tcp6sk = (struct tcp6_sock *)foo;
+
+	tcp6sk->pinet6 = &tcp6sk->inet6;
+}
+
+static void udp6_pinet6_init(void *foo, kmem_cache_t *cachep,
+			     unsigned long flags)
+{
+	struct udp6_sock *udp6sk = (struct udp6_sock *)foo;
+
+	udp6sk->pinet6 = &udp6sk->inet6;
+}
+
+static void raw6_pinet6_init(void *foo, kmem_cache_t *cachep,
+			     unsigned long flags)
+{
+	struct raw6_sock *raw6sk = (struct raw6_sock *)foo;
+
+	raw6sk->pinet6 = &raw6sk->inet6;
+}
+
 static int __init inet6_init(void)
 {
 	struct sk_buff *dummy_skb;
@@ -655,13 +666,16 @@
 	/* allocate our sock slab caches */
         tcp6_sk_cachep = kmem_cache_create("tcp6_sock",
 					   sizeof(struct tcp6_sock), 0,
-                                           SLAB_HWCACHE_ALIGN, 0, 0);
+                                           SLAB_HWCACHE_ALIGN,
+					   tcp6_pinet6_init, NULL);
         udp6_sk_cachep = kmem_cache_create("udp6_sock",
 					   sizeof(struct udp6_sock), 0,
-                                           SLAB_HWCACHE_ALIGN, 0, 0);
+                                           SLAB_HWCACHE_ALIGN,
+					   udp6_pinet6_init, NULL);
         raw6_sk_cachep = kmem_cache_create("raw6_sock",
 					   sizeof(struct raw6_sock), 0,
-                                           SLAB_HWCACHE_ALIGN, 0, 0);
+                                           SLAB_HWCACHE_ALIGN,
+					   raw6_pinet6_init, NULL);
         if (!tcp6_sk_cachep || !udp6_sk_cachep || !raw6_sk_cachep)
                 printk(KERN_CRIT __FUNCTION__
                         ": Can't create protocol sock SLAB caches!\n");
diff -Nru a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
--- a/net/ipv6/tcp_ipv6.c	Wed May 22 15:16:37 2002
+++ b/net/ipv6/tcp_ipv6.c	Tue Jun 18 00:27:30 2002
@@ -1260,6 +1260,8 @@
 		newnp = inet6_sk(newsk);
 		newtp = tcp_sk(newsk);
 
+		memcpy(newnp, np, sizeof(struct ipv6_pinfo));
+
 		ipv6_addr_set(&newnp->daddr, 0, 0, htonl(0x0000FFFF),
 			      newinet->daddr);
 
@@ -1339,6 +1341,7 @@
 	newtp = tcp_sk(newsk);
 	newinet = inet_sk(newsk);
 	newnp = inet6_sk(newsk);
+	memcpy(newnp, np, sizeof(struct ipv6_pinfo));
 	ipv6_addr_copy(&newnp->daddr, &req->af.v6_req.rmt_addr);
 	ipv6_addr_copy(&newnp->saddr, &req->af.v6_req.loc_addr);
 	ipv6_addr_copy(&newnp->rcv_saddr, &req->af.v6_req.loc_addr);

Re: [BKPATCH] Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Arnaldo Carvalho de Melo]

Em Tue, Jun 18, 2002 at 12:58:04AM -0300, Arnaldo C. Melo escreveu:
> Em Mon, Jun 17, 2002 at 11:49:34PM -0300, Arnaldo C. Melo escreveu:
> > Em Mon, Jun 17, 2002 at 07:17:26PM -0700, David S. Miller escreveu:
> > >    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
> > >    Date: Mon, 17 Jun 2002 21:57:35 -0300
> > > 
> > >    --- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
> > >    +++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002
> > > 
> > > I've installed this change into my tree in the meantime.
> > > If we find a better fix, we can just revert this.
> > 
> > OK, I found a better fix, I think, that allowed me to kill inet6_sk_generic
> > in af_inet6.c, using a constructor for the tcpv6, udpv6 and raw6 slab caches,
> > as suggested by Russel King, I'll be sending RSN.
> 
> Here it is, David, please consider pulling it from:
> 
> http://kernel-acme.bkbits.net:8080/tcpv6-pinet6
> 
> Best Regards,
> 
> - Arnaldo

Oops, brain fart, David, please don't apply, in tcp_create_openreq_child
we overwrite the ->pinet6 field after the constructor inits it to the proper
value :( Please leave the old patch in place for a while... :(

- Arnaldo

Re: [BKPATCH] Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[David S. Miller]

   From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
   Date: Tue, 18 Jun 2002 01:15:39 -0300
   
   Oops, brain fart, David, please don't apply, in tcp_create_openreq_child
   we overwrite the ->pinet6 field after the constructor inits it to the proper
   value :( Please leave the old patch in place for a while... :(

No problem.

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Carl Ritson]

On Mon, 17 Jun 2002, Arnaldo Carvalho de Melo wrote:

> Em Mon, Jun 17, 2002 at 02:33:19PM -0700, David S. Miller escreveu:
> > 
> > This is a known bug introduced by the struct sock splitup into
> > external per-protocol pieces done by Arnaldo de Melo.  He is working
> > on the proper fix, your proposed change will just paper over the real
> > bug.

I suspected something like that.

> Carl,
> 
> 	Can you try this patch?
> 
> - Arnaldo
> 
> --- orig/net/ipv6/tcp_ipv6.c    Sat May 25 23:13:56 2002
> +++ linux/net/ipv6/tcp_ipv6.c   Fri Jun 14 23:23:07 2002
> <diff snipped> 

This patch doesn't help, it produces exactly the same oops when decoded, I 
assume the newer patch you mention to Dave is the correct fix?

Thanks,
Carl

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[kuznet]

Hello!

> This is a known bug introduced by the struct sock splitup into
> external per-protocol pieces done by Arnaldo de Melo.  He is working
> on the proper fix, your proposed change will just paper over the real
> bug.

I am afraid you speak about different bug.

Old code really relied on the fact that each IPv4 code has zeroed IPv6
part, and that each IPv6 socket has correctly intialized IPv4 part.

So, Carl's suggestion may be right. Each time when accesing IPv6 data
we have to check that it really exists (when we are not sure).

Alexey

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[David S. Miller]

   From: kuznet@ms2.inr.ac.ru
   Date: Tue, 18 Jun 2002 13:43:21 +0400 (MSD)
   
   I am afraid you speak about different bug.

It is different bug, indeed.  The oops looked very similar, it
tricked me :-)

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Arnaldo Carvalho de Melo]

Em Tue, Jun 18, 2002 at 02:58:11AM -0700, David S. Miller escreveu:
>    From: kuznet@ms2.inr.ac.ru
>    Date: Tue, 18 Jun 2002 13:43:21 +0400 (MSD)
>    
>    I am afraid you speak about different bug.
> 
> It is different bug, indeed.  The oops looked very similar, it
> tricked me :-)

I'll be looking at this while in Ottawa 8)

- Arnaldo

net/ipv6/exthdrs.c

[Andras Kis-Szabo]

Hi,

It's just a short question...

Is there any plan to add the ESP header to the ipv6_ext_hdr() function (as a
known header)?
(It requires changes in this file and in the icmp.c at the first round.)

Some user noticed that the Netfilter contains a very similar function, which
knows the ESP.
If this functon remains untouched, I'll add an another one into the Netfilter
code (which can be used at the special matches).

Regards,

	kisza

-- 
    Andras Kis-Szabo       Security Development, Design and Audit
-------------------------/        Zorp, NetFilter and IPv6
 kisza@SecurityAudit.hu /-----Member of the BUTE-MIS-SEARCHlab---------->

Re: net/ipv6/exthdrs.c

[Pekka Savola]

On Tue, 18 Jun 2002, Andras Kis-Szabo wrote:
> Is there any plan to add the ESP header to the ipv6_ext_hdr() function (as a
> known header)?
> (It requires changes in this file and in the icmp.c at the first round.)

Quickly looking at it, I don't know if adding it would help any (on the 
countrary).

The code seems to be used mainly to skip over extension headers
(forbidden, strictly speaking) when generating ICMP messages; in the case 
of ESP, the rest of the payload should be encrypted so adding it to the 
list would probably not change anything?

-- 
Pekka Savola                 "Tell me of difficulties surmounted,
Netcore Oy                   not those you stumble over and fall"
Systems. Networks. Security.  -- Robert Jordan: A Crown of Swords

Re: net/ipv6/exthdrs.c

[Andras Kis-Szabo]

Pekka Savola ........................................ (2002. június 18.)

 Hi!

> > Is there any plan to add the ESP header to the ipv6_ext_hdr() function (as a
> > known header)?
> > (It requires changes in this file and in the icmp.c at the first round.)
> Quickly looking at it, I don't know if adding it would help any (on the 
> countrary).
At the firewall side the ESP is a known extension header. The ESP contains
some field which can be parsed in a strict firewall rule.
When the extension headers and the main header parsed by the Netfilter, the
upper level protocol should be passed to the next level for future parsing.
The implementation follows the standard where the ESP is one of the extension
headers.
BTW, the Netfilter code can be changed to this behaviour. (Minor changes in
some file and a major change in the ESP match.)

The ipv6_ext_hdr() could be exported? It would be usefull at the Netfilter
side.
(And when we are there: the ipv6_skip_exthdr() should be exported, too.)

> The code seems to be used mainly to skip over extension headers
> (forbidden, strictly speaking) when generating ICMP messages; in the case 
> of ESP, the rest of the payload should be encrypted so adding it to the 
> list would probably not change anything?
At first look in the ipv6_skip_exthdr() in the parser loop:
-                if (nexthdr == NEXTHDR_NONE)
+                if ( (nexthdr == NEXTHDR_NONE) || (nexthdr == NEXTHDR_ESP) )
But after this change the ICMPv6 reply won't contain the ESP ...

Regards,

	kisza

-- 
    Andras Kis-Szabo       Security Development, Design and Audit
-------------------------/        Zorp, NetFilter and IPv6
 kisza@SecurityAudit.hu /-----Member of the BUTE-MIS-SEARCHlab---------->

[PATCH] Re: net/ipv6/exthdrs.c

[Andras Kis-Szabo]

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

Andras Kis-Szabo .................................... (2002. június 18.)

 Hi!


> The ipv6_ext_hdr() could be exported? It would be usefull at the Netfilter
> side.
> (And when we are there: the ipv6_skip_exthdr() should be exported, too.)
I attached the patch for this. (If accepted, the Netfilter will follow its
behaviour and I'll modify the matches.)

Regards,

	kisza

-- 
    Andras Kis-Szabo       Security Development, Design and Audit
-------------------------/        Zorp, NetFilter and IPv6
 kisza@SecurityAudit.hu /-----Member of the BUTE-MIS-SEARCHlab---------->

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 422 bytes --]

--- linux.old/net/ipv6/exthdrs.c	Tue Jun 18 15:54:19 2002
+++ linux/net/ipv6/exthdrs.c	Tue Jun 18 15:55:34 2002
@@ -20,6 +20,8 @@
  *				  tlv options.
  */
 
+#include <linux/config.h>
+#include <linux/module.h>
 #include <linux/errno.h>
 #include <linux/types.h>
 #include <linux/socket.h>
@@ -804,4 +806,7 @@
 	*nexthdrp = nexthdr;
 	return start;
 }
+
+EXPORT_SYMBOL(ipv6_ext_hdr);
+EXPORT_SYMBOL(ipv6_skip_exthdr);
 

Re: net/ipv6/exthdrs.c

[kuznet]

Hello!

> Is there any plan to add the ESP header to the ipv6_ext_hdr() function (as a
> known header)?

No, ESP is not a normal extension header, it terminates parse.
So, ipv6_skip_headers cannot skip it.

BTW the same is with netfilter. I do not see how are you going to use it. :-)

> (It requires changes in this file and in the icmp.c at the first round.)

I am afraid this will simply break the function.


> If this functon remains untouched, I'll add an another one into the Netfilter
> code (which can be used at the special matches).

This may be right even not depending on this issue. Goals are different:
the function in exthdrs.c does the best efforts to guess what protocol
is, the function in netfilter should be paranoid.

Alexey

Re: net/ipv6/exthdrs.c

[Kis-Szabo Andras]

Hello,

> > Is there any plan to add the ESP header to the ipv6_ext_hdr() function (as a
> > known header)?
> No, ESP is not a normal extension header, it terminates parse.
> So, ipv6_skip_headers cannot skip it.
The same behaviour as in NONE, but the NONE is listed and the ESP is
not. (But it is not a problem to me, I just asked something :) )

> BTW the same is with netfilter. I do not see how are you going to use it. :-)
The ESP belongs to the headers, it is a member of a possible chain.
- header match - i had to search for the ESP, too
- ESP match - it has a public SPI value, which can be used in rules
- general iteration, skipped together with the NONE.
It terminates the header chain, but the existance of the ESP header and
its SPI value are usefull information.

> > (It requires changes in this file and in the icmp.c at the first round.)
> I am afraid this will simply break the function.
Yes, i am afraid You're right. :(
Adding the ESP to the headers will break the icmp code. :(

> This may be right even not depending on this issue. Goals are different:
> the function in exthdrs.c does the best efforts to guess what protocol
> is, the function in netfilter should be paranoid.
I added a similar function (exactly the same but with the ESP) to decide
about the nexthdr value and a new header parser/evaluator with strict
size/pointer checks.
Last week one of our user sent a direct request to eliminate the
duplicated functions - so He pushed me to send the original question to
this forum.

Thanks for the answers, I 'wrote up them'.

Regards,

	kisza

-- 
    Andras Kis-Szabo       Security Development, Design and Audit
-------------------------/        Zorp, NetFilter and IPv6
 kisza@SecurityAudit.hu /-----Member of the BUTE-MIS-SEARCHlab------>

Re: net/ipv6/exthdrs.c

[kuznet]

Hello!

> The same behaviour as in NONE,

Yes, this is equivalent. And the function behaves exactly as you
expect then. :-)


> The ESP belongs to the headers, it is a member of a possible chain.

Like UDP, TCP (and NONE as you noticed). Not like headers mentioned
in ipv6_ext_hdr().


> - header match - i had to search for the ESP, too
> - ESP match - it has a public SPI value, which can be used in rules
> - general iteration, skipped together with the NONE.
> It terminates the header chain, but the existance of the ESP header and
> its SPI value are usefull information.

Hey, we spoke about _skip_. For all items mentioned by you ESP
is considered like UDP&TCP, not like extension header!


> about the nexthdr value and a new header parser/evaluator with strict
> size/pointer checks.

I feel you make something wrong yet. Or I do not understand something.

Alexey