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

[Vlad Yasevich <vladislav.yasevich@hp.com>]

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