[PATCH][2.5.22] OOPS in tcp_v6_get_port

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

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.

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

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

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

 Hi!

quoted

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

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

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