[PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Dave Johnson]

An accept() call on a SCTPv6 socket that returns due to connection of
a IPv4 mapped peer will fill out the 'struct sockaddr' with a zero
IPv6 address instead of the IPv4 mapped address of the peer.

This is due to the v4mapped flag not getting copied into the new
socket on accept() as well as a missing check for INET6 socket type in
sctp_v4_to_sk_*addr().

Signed-off-by: Dave Johnson <djohnson+linux-kernel@sw.starentnetworks.com>
Cc: Srinivas Akkipeddi <sakkiped@starentnetworks.com>

===== net/sctp/ipv6.c 1.108 vs edited =====
--- 1.108/net/sctp/ipv6.c	2007-07-05 20:40:15 -04:00
+++ edited/net/sctp/ipv6.c	2007-07-25 16:30:41 -04:00
@@ -641,6 +641,8 @@
 	newsctp6sk = (struct sctp6_sock *)newsk;
 	inet_sk(newsk)->pinet6 = &newsctp6sk->inet6;
 
+	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
+
 	newinet = inet_sk(newsk);
 	newnp = inet6_sk(newsk);
 
===== net/sctp/protocol.c 1.130 vs edited =====
--- 1.130/net/sctp/protocol.c	2007-05-04 16:36:30 -04:00
+++ edited/net/sctp/protocol.c	2007-07-25 16:28:21 -04:00
@@ -257,13 +257,28 @@
 /* Initialize sk->sk_rcv_saddr from sctp_addr. */
 static void sctp_v4_to_sk_saddr(union sctp_addr *addr, struct sock *sk)
 {
-	inet_sk(sk)->rcv_saddr = addr->v4.sin_addr.s_addr;
+	if ((sk->sk_family == PF_INET6) && (sctp_sk(sk)->v4mapped)) {
+		inet6_sk(sk)->rcv_saddr.s6_addr32[0] = 0;
+		inet6_sk(sk)->rcv_saddr.s6_addr32[1] = 0;
+		inet6_sk(sk)->rcv_saddr.s6_addr32[2] = htonl(0x0000ffff);
+		inet6_sk(sk)->rcv_saddr.s6_addr32[3] =
+			addr->v4.sin_addr.s_addr;
+	} else {
+		inet_sk(sk)->rcv_saddr = addr->v4.sin_addr.s_addr;
+	}
 }
 
 /* Initialize sk->sk_daddr from sctp_addr. */
 static void sctp_v4_to_sk_daddr(union sctp_addr *addr, struct sock *sk)
 {
-	inet_sk(sk)->daddr = addr->v4.sin_addr.s_addr;
+	if ((sk->sk_family == PF_INET6) && (sctp_sk(sk)->v4mapped)) {
+		inet6_sk(sk)->daddr.s6_addr32[0] = 0;
+		inet6_sk(sk)->daddr.s6_addr32[1] = 0;
+		inet6_sk(sk)->daddr.s6_addr32[2] = htonl(0x0000ffff);
+		inet6_sk(sk)->daddr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
+	} else {
+		inet_sk(sk)->daddr = addr->v4.sin_addr.s_addr;
+	}
 }
 
 /* Initialize a sctp_addr from an address parameter. */
@@ -557,6 +572,8 @@
 	newsk->sk_protocol = IPPROTO_SCTP;
 	newsk->sk_backlog_rcv = sk->sk_prot->backlog_rcv;
 	sock_reset_flag(newsk, SOCK_ZAPPED);
+
+	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
 
 	newinet = inet_sk(newsk);
 

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

Dave Johnson wrote:
> An accept() call on a SCTPv6 socket that returns due to connection of
> a IPv4 mapped peer will fill out the 'struct sockaddr' with a zero
> IPv6 address instead of the IPv4 mapped address of the peer.
> 
> This is due to the v4mapped flag not getting copied into the new
> socket on accept() as well as a missing check for INET6 socket type in
> sctp_v4_to_sk_*addr().
> 
> Signed-off-by: Dave Johnson <djohnson+linux-kernel@sw.starentnetworks.com>
> Cc: Srinivas Akkipeddi <sakkiped@starentnetworks.com>
> 
> ===== net/sctp/ipv6.c 1.108 vs edited =====
> --- 1.108/net/sctp/ipv6.c	2007-07-05 20:40:15 -04:00
> +++ edited/net/sctp/ipv6.c	2007-07-25 16:30:41 -04:00
> @@ -641,6 +641,8 @@
>  	newsctp6sk = (struct sctp6_sock *)newsk;
>  	inet_sk(newsk)->pinet6 = &newsctp6sk->inet6;
>  
> +	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
> +
>  	newinet = inet_sk(newsk);
>  	newnp = inet6_sk(newsk);
>  
> ===== net/sctp/protocol.c 1.130 vs edited =====
> --- 1.130/net/sctp/protocol.c	2007-05-04 16:36:30 -04:00
> +++ edited/net/sctp/protocol.c	2007-07-25 16:28:21 -04:00
> @@ -257,13 +257,28 @@
>  /* Initialize sk->sk_rcv_saddr from sctp_addr. */
>  static void sctp_v4_to_sk_saddr(union sctp_addr *addr, struct sock *sk)
>  {
> -	inet_sk(sk)->rcv_saddr = addr->v4.sin_addr.s_addr;
> +	if ((sk->sk_family == PF_INET6) && (sctp_sk(sk)->v4mapped)) {
> +		inet6_sk(sk)->rcv_saddr.s6_addr32[0] = 0;
> +		inet6_sk(sk)->rcv_saddr.s6_addr32[1] = 0;
> +		inet6_sk(sk)->rcv_saddr.s6_addr32[2] = htonl(0x0000ffff);
> +		inet6_sk(sk)->rcv_saddr.s6_addr32[3] =
> +			addr->v4.sin_addr.s_addr;
> +	} else {
> +		inet_sk(sk)->rcv_saddr = addr->v4.sin_addr.s_addr;
> +	}
>  }
>  
>  /* Initialize sk->sk_daddr from sctp_addr. */
>  static void sctp_v4_to_sk_daddr(union sctp_addr *addr, struct sock *sk)
>  {
> -	inet_sk(sk)->daddr = addr->v4.sin_addr.s_addr;
> +	if ((sk->sk_family == PF_INET6) && (sctp_sk(sk)->v4mapped)) {
> +		inet6_sk(sk)->daddr.s6_addr32[0] = 0;
> +		inet6_sk(sk)->daddr.s6_addr32[1] = 0;
> +		inet6_sk(sk)->daddr.s6_addr32[2] = htonl(0x0000ffff);
> +		inet6_sk(sk)->daddr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
> +	} else {
> +		inet_sk(sk)->daddr = addr->v4.sin_addr.s_addr;
> +	}
>  }
>  
>  /* Initialize a sctp_addr from an address parameter. */
> @@ -557,6 +572,8 @@
>  	newsk->sk_protocol = IPPROTO_SCTP;
>  	newsk->sk_backlog_rcv = sk->sk_prot->backlog_rcv;
>  	sock_reset_flag(newsk, SOCK_ZAPPED);
> +
> +	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
>  
>  	newinet = inet_sk(newsk);
>  
> 

Can you explain why the sctp_v4 changes are need for the this case?
I don't see how the code in sctp/protocol.c comes into play for this
particular bug.

Thanks
-vlad

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Dave Johnson]

Vlad Yasevich writes:
> Can you explain why the sctp_v4 changes are need for the this case?
> I don't see how the code in sctp/protocol.c comes into play for this
> particular bug.

connect() on v6 socket to v4 mapped address will trigger
sctp_v4_to_sk_daddr:

strace:

socket(PF_INET6, SOCK_STREAM, 0x84 /* IPPROTO_??? */) = 3
bind(3, {sa_family=AF_INET6, sin6_port=htons(0), inet_pton(AF_INET6, "::", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, 128) = 0
connect(3, {sa_family=AF_INET6, sin6_port=htons(33333), inet_pton(AF_INET6, "::ffff:192.168.207.231", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, 128) = 0
getsockname(3, {sa_family=AF_INET6, sin6_port=htons(32771), inet_pton(AF_INET6, "::ffff:192.168.207.234", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 0
getpeername(3, {sa_family=AF_INET6, sin6_port=htons(33333), inet_pton(AF_INET6, "::ffff:192.168.207.231", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 0

stack dump:

 [<f8c539de>] sctp_v4_to_sk_daddr+0x3e/0x80 [sctp]
 [<f8c5f6c4>] __sctp_connect+0x2a4/0x520 [sctp]
 [<f8c61810>] sctp_connect+0x60/0x70 [sctp]
 [<c02c4b2c>] inet_dgram_connect+0x4c/0x80
 [<c026dbab>] sys_connect+0x8b/0xd0
 [<c026e711>] sys_socketcall+0xb1/0x260
 [<c0103013>] syscall_call+0x7/0xb

I'm unsure if there is a path to call sctp_v4_to_sk_saddr() but it was
added just to be complete.

v4mapped in sctp_v4_create_accept_sk() probably isn't needed, but
since v4mapped is in sctp_sock not sctp6_sock copying it seems like a
good idea.

-- 
Dave Johnson
Starent Networks

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

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

Dave Johnson wrote:
> Vlad Yasevich writes:
>> Can you explain why the sctp_v4 changes are need for the this case?
>> I don't see how the code in sctp/protocol.c comes into play for this
>> particular bug.
> 
> connect() on v6 socket to v4 mapped address will trigger
> sctp_v4_to_sk_daddr:
> 
> strace:
> 
> socket(PF_INET6, SOCK_STREAM, 0x84 /* IPPROTO_??? */) = 3
> bind(3, {sa_family=AF_INET6, sin6_port=htons(0), inet_pton(AF_INET6, "::", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, 128) = 0
> connect(3, {sa_family=AF_INET6, sin6_port=htons(33333), inet_pton(AF_INET6, "::ffff:192.168.207.231", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, 128) = 0
> getsockname(3, {sa_family=AF_INET6, sin6_port=htons(32771), inet_pton(AF_INET6, "::ffff:192.168.207.234", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 0
> getpeername(3, {sa_family=AF_INET6, sin6_port=htons(33333), inet_pton(AF_INET6, "::ffff:192.168.207.231", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 0
> 
> stack dump:
> 
>  [<f8c539de>] sctp_v4_to_sk_daddr+0x3e/0x80 [sctp]
>  [<f8c5f6c4>] __sctp_connect+0x2a4/0x520 [sctp]
>  [<f8c61810>] sctp_connect+0x60/0x70 [sctp]
>  [<c02c4b2c>] inet_dgram_connect+0x4c/0x80
>  [<c026dbab>] sys_connect+0x8b/0xd0
>  [<c026e711>] sys_socketcall+0xb1/0x260
>  [<c0103013>] syscall_call+0x7/0xb
> 
> I'm unsure if there is a path to call sctp_v4_to_sk_saddr() but it was
> added just to be complete.
> 
> v4mapped in sctp_v4_create_accept_sk() probably isn't needed, but
> since v4mapped is in sctp_sock not sctp6_sock copying it seems like a
> good idea.

Ok.  First, this is a different bug, so I would prefer a separate patch.
Also, I see the problem and it's ugly, but this solution is not really correct,
both conceptually and code wise.

Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
muck with them.  They are IPv6 addresses and there should be a clean separation.

Code wise, the code in the __sctp_connect() is terrible.

Does the attached patch work for you in this case.

Thanks
-vlad

[-- Attachment #2: connect_bug.txt --]
[-- Type: text/plain, Size: 2544 bytes --]

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index b1917f6..1577814 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -977,7 +977,7 @@ static int __sctp_connect(struct sock* sk,
 	int err = 0;
 	int addrcnt = 0;
 	int walk_size = 0;
-	union sctp_addr *sa_addr;
+	union sctp_addr *sa_addr = NULL;
 	void *addr_buf;
 	unsigned short port;
 	unsigned int f_flags = 0;
@@ -1011,7 +1011,10 @@ static int __sctp_connect(struct sock* sk,
 			goto out_free;
 		}
 
-		err = sctp_verify_addr(sk, sa_addr, af->sockaddr_len);
+		/* Save current address so we can work with it */
+		memcpy(&to, sa_addr, af->sockaddr_len);
+
+		err = sctp_verify_addr(sk, &to, af->sockaddr_len);
 		if (err)
 			goto out_free;
 
@@ -1021,12 +1024,11 @@ static int __sctp_connect(struct sock* sk,
 		if (asoc && asoc->peer.port && asoc->peer.port != port)
 			goto out_free;
 
-		memcpy(&to, sa_addr, af->sockaddr_len);
 
 		/* Check if there already is a matching association on the
 		 * endpoint (other than the one created here).
 		 */
-		asoc2 = sctp_endpoint_lookup_assoc(ep, sa_addr, &transport);
+		asoc2 = sctp_endpoint_lookup_assoc(ep, &to, &transport);
 		if (asoc2 && asoc2 != asoc) {
 			if (asoc2->state >= SCTP_STATE_ESTABLISHED)
 				err = -EISCONN;
@@ -1039,7 +1041,7 @@ static int __sctp_connect(struct sock* sk,
 		 * make sure that there is no peeled-off association matching
 		 * the peer address even on another socket.
 		 */
-		if (sctp_endpoint_is_peeled_off(ep, sa_addr)) {
+		if (sctp_endpoint_is_peeled_off(ep, &to)) {
 			err = -EADDRNOTAVAIL;
 			goto out_free;
 		}
@@ -1070,7 +1072,7 @@ static int __sctp_connect(struct sock* sk,
 				}
 			}
 
-			scope = sctp_scope(sa_addr);
+			scope = sctp_scope(&to);
 			asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
 			if (!asoc) {
 				err = -ENOMEM;
@@ -1079,7 +1081,7 @@ static int __sctp_connect(struct sock* sk,
 		}
 
 		/* Prime the peer's transport structures.  */
-		transport = sctp_assoc_add_peer(asoc, sa_addr, GFP_KERNEL,
+		transport = sctp_assoc_add_peer(asoc, &to, GFP_KERNEL,
 						SCTP_UNKNOWN);
 		if (!transport) {
 			err = -ENOMEM;
@@ -1103,8 +1105,8 @@ static int __sctp_connect(struct sock* sk,
 
 	/* Initialize sk's dport and daddr for getpeername() */
 	inet_sk(sk)->dport = htons(asoc->peer.port);
-	af = sctp_get_af_specific(to.sa.sa_family);
-	af->to_sk_daddr(&to, sk);
+	af = sctp_get_af_specific(sa_addr->sa.sa_family);
+	af->to_sk_daddr(sa_addr, sk);
 	sk->sk_err = 0;
 
 	/* in-kernel sockets don't generally have a file allocated to them

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Dave Johnson]

Vlad Yasevich writes:
> Ok.  First, this is a different bug, so I would prefer a separate patch.
> Also, I see the problem and it's ugly, but this solution is not really correct,
> both conceptually and code wise.
> 
> Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
> muck with them.  They are IPv6 addresses and there should be a clean separation.
> 
> Code wise, the code in the __sctp_connect() is terrible.
> 
> Does the attached patch work for you in this case.

yes, with the v4mapped in ipv6.c and your patch, connect and accept
both work with v4 mapped addresses.

Note instead of:

> +	af = sctp_get_af_specific(sa_addr->sa.sa_family);
> +	af->to_sk_daddr(sa_addr, sk);

you should have:

> +	af = sctp_get_af_specific(sa_addr->sa_family);
> +	af->to_sk_daddr((union sctp_addr *)sa_addr, sk);


-- 
Dave Johnson
Starent Networks

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

Dave Johnson wrote:
> Vlad Yasevich writes:
>> Ok.  First, this is a different bug, so I would prefer a separate patch.
>> Also, I see the problem and it's ugly, but this solution is not really correct,
>> both conceptually and code wise.
>>
>> Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
>> muck with them.  They are IPv6 addresses and there should be a clean separation.
>>
>> Code wise, the code in the __sctp_connect() is terrible.
>>
>> Does the attached patch work for you in this case.
> 
> yes, with the v4mapped in ipv6.c and your patch, connect and accept
> both work with v4 mapped addresses.
> 
> Note instead of:
> 
>> +	af = sctp_get_af_specific(sa_addr->sa.sa_family);
>> +	af->to_sk_daddr(sa_addr, sk);
> 
> you should have:
> 
>> +	af = sctp_get_af_specific(sa_addr->sa_family);
>> +	af->to_sk_daddr((union sctp_addr *)sa_addr, sk);
> 
> 

Feel free to clean it up and submit both patches.

-vlad

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 26 Jul 2007 16:49:40 -0400

> Feel free to clean it up and submit both patches.

Ping?  Somebody?

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

David Miller wrote:
> From: Vlad Yasevich <vladislav.yasevich@hp.com>
> Date: Thu, 26 Jul 2007 16:49:40 -0400
> 
>> Feel free to clean it up and submit both patches.
> 
> Ping?  Somebody?
> 

I am head deep in an ugly bug.  I might take a break
and do this mindless excercise later today if nobody beats
me to it.

-vlad

[PATCH] SCTP: fix IPv4 addr in SCTPv6 accept()/getpeername()

[Dave Johnson]

An accept() call on a SCTPv6 socket that returns due to connection of
a IPv4 mapped peer will fill out the 'struct sockaddr' with a zero
IPv6 address instead of the IPv4 mapped address of the peer.

This is due to the v4mapped flag not getting copied into the new
socket on accept().

Signed-off-by: Dave Johnson <djohnson@sw.starentnetworks.com>
Cc: Srinivas Akkipeddi <sakkiped@starentnetworks.com>

===== net/sctp/ipv6.c 1.108 vs edited =====
--- 1.108/net/sctp/ipv6.c	2007-07-05 20:40:15 -04:00
+++ edited/net/sctp/ipv6.c	2007-07-25 16:30:41 -04:00
@@ -641,6 +641,8 @@
 	newsctp6sk = (struct sctp6_sock *)newsk;
 	inet_sk(newsk)->pinet6 = &newsctp6sk->inet6;
 
+	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
+
 	newinet = inet_sk(newsk);
 	newnp = inet6_sk(newsk);
 

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Dave Johnson]

Vlad Yasevich writes:
> Ok.  First, this is a different bug, so I would prefer a separate patch.
> Also, I see the problem and it's ugly, but this solution is not really correct,
> both conceptually and code wise.
> 
> Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
> muck with them.  They are IPv6 addresses and there should be a clean separation.
> 
> Code wise, the code in the __sctp_connect() is terrible.
> 
> Does the attached patch work for you in this case.

Sorry about the confusion before, your patch to __sctp_connect()
fixes calls to getpeername() after connect() just fine.

I'll post a patch for the accept()/getpeername() case in a bit.

Acked-by: Dave Johnson <djohnson@sw.starentnetworks.com>

-- 
Dave Johnson
Starent Networks

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

Dave Johnson wrote:
> Vlad Yasevich writes:
>> Ok.  First, this is a different bug, so I would prefer a separate patch.
>> Also, I see the problem and it's ugly, but this solution is not really correct,
>> both conceptually and code wise.
>>
>> Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
>> muck with them.  They are IPv6 addresses and there should be a clean separation.
>>
>> Code wise, the code in the __sctp_connect() is terrible.
>>
>> Does the attached patch work for you in this case.
> 
> Sorry about the confusion before, your patch to __sctp_connect()
> fixes calls to getpeername() after connect() just fine.
> 
> I'll post a patch for the accept()/getpeername() case in a bit.
> 
> Acked-by: Dave Johnson <djohnson@sw.starentnetworks.com>
> 

I had time to tinker and pushed both patches already.  They were in
the last pull that Dave Miller did.  I also tested both patches locally
to make sure that they worked as supposed to.

-vlad