[PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[Jason Gunthorpe]

sctp_v4_map_v6 was subtly writing and reading from members
of a union in a way the clobbered data it needed to read before
it read it.

Zeroing the v6 flowinfo overwrites the v4 sin_addr with 0, meaning
that every place that calls sctp_v4_map_v6 gets ::ffff:0.0.0.0 as the
result.

Reorder things to guarantee correct behaviour no matter what the
union layout is.

This impacts user space clients that open an IPv6 SCTP socket and
receive IPv4 connections. Prior to 299ee user space would see a
sockaddr with AF_INET and a correct address, after 299ee the sockaddr
is AF_INET6, but the address is wrong.

Fixes: 299ee123e198 (sctp: Fixup v4mapped behaviour to comply with Sock API)
Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 include/net/sctp/sctp.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

This bugfix should be a candidate for -stable

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 856f01cb51dd..230775f5952a 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -571,11 +571,14 @@ static inline void sctp_v6_map_v4(union sctp_addr *addr)
 /* Map v4 address to v4-mapped v6 address */
 static inline void sctp_v4_map_v6(union sctp_addr *addr)
 {
+	__be16 port;
+
+	port = addr->v4.sin_port;
+	addr->v6.sin6_addr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
+	addr->v6.sin6_port = port;
 	addr->v6.sin6_family = AF_INET6;
 	addr->v6.sin6_flowinfo = 0;
 	addr->v6.sin6_scope_id = 0;
-	addr->v6.sin6_port = addr->v4.sin_port;
-	addr->v6.sin6_addr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
 	addr->v6.sin6_addr.s6_addr32[0] = 0;
 	addr->v6.sin6_addr.s6_addr32[1] = 0;
 	addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
-- 
2.1.4

Re: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[Daniel Borkmann]

On 05/27/2015 01:30 AM, Jason Gunthorpe wrote:
> sctp_v4_map_v6 was subtly writing and reading from members
> of a union in a way the clobbered data it needed to read before

s/the/that/

> it read it.
>
> Zeroing the v6 flowinfo overwrites the v4 sin_addr with 0, meaning
> that every place that calls sctp_v4_map_v6 gets ::ffff:0.0.0.0 as the
> result.
>
> Reorder things to guarantee correct behaviour no matter what the
> union layout is.
>
> This impacts user space clients that open an IPv6 SCTP socket and
> receive IPv4 connections. Prior to 299ee user space would see a
> sockaddr with AF_INET and a correct address, after 299ee the sockaddr
> is AF_INET6, but the address is wrong.
>
> Fixes: 299ee123e198 (sctp: Fixup v4mapped behaviour to comply with Sock API)
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>   include/net/sctp/sctp.h | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> This bugfix should be a candidate for -stable
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 856f01cb51dd..230775f5952a 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -571,11 +571,14 @@ static inline void sctp_v6_map_v4(union sctp_addr *addr)
>   /* Map v4 address to v4-mapped v6 address */
>   static inline void sctp_v4_map_v6(union sctp_addr *addr)
>   {
> +	__be16 port;
> +
> +	port = addr->v4.sin_port;
> +	addr->v6.sin6_addr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
> +	addr->v6.sin6_port = port;
>   	addr->v6.sin6_family = AF_INET6;
>   	addr->v6.sin6_flowinfo = 0;
>   	addr->v6.sin6_scope_id = 0;
> -	addr->v6.sin6_port = addr->v4.sin_port;
> -	addr->v6.sin6_addr.s6_addr32[3] = addr->v4.sin_addr.s_addr;

Change looks good to me. You actually would only need to address
the issue where the v4.sin_addr is copied before you overwrite/zero
the flowinfo as only these two overlap in the union. Given that
sockaddr_in and sockaddr_in6 are exported to uapi, they are immutable,
but I see the point that union sctp_addr is kernel private - bad
things happen if v4/v6 don't overlap the way anymore as they'd do
now. ;)

Anyways:

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

>   	addr->v6.sin6_addr.s6_addr32[0] = 0;
>   	addr->v6.sin6_addr.s6_addr32[1] = 0;
>   	addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>

RE: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[David Laight]

From: Jason Gunthorpe
> Sent: 27 May 2015 00:30
> sctp_v4_map_v6 was subtly writing and reading from members
> of a union in a way the clobbered data it needed to read before
> it read it.
> 
> Zeroing the v6 flowinfo overwrites the v4 sin_addr with 0, meaning
> that every place that calls sctp_v4_map_v6 gets ::ffff:0.0.0.0 as the
> result.
> 
> Reorder things to guarantee correct behaviour no matter what the
> union layout is.
> 
> This impacts user space clients that open an IPv6 SCTP socket and
> receive IPv4 connections. Prior to 299ee user space would see a
> sockaddr with AF_INET and a correct address, after 299ee the sockaddr
> is AF_INET6, but the address is wrong.
> 
> Fixes: 299ee123e198 (sctp: Fixup v4mapped behaviour to comply with Sock API)
...
> This bugfix should be a candidate for -stable

Anyone know off-hand which kernel releases are affected?
I'm going to have to note this in the release notes for one of our products.

	David

Re: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[Daniel Borkmann]

On 05/27/2015 11:06 AM, David Laight wrote:
> From: Jason Gunthorpe
...
>> Fixes: 299ee123e198 (sctp: Fixup v4mapped behaviour to comply with Sock API)
> ...
>> This bugfix should be a candidate for -stable
>
> Anyone know off-hand which kernel releases are affected?
> I'm going to have to note this in the release notes for one of our products.

Ever heard of git-describe(1) ? ;)

git describe 299ee123e19889d511092347f5fc14db0f10e3a6
v3.16-rc7-1525-g299ee12

RE: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[David Laight]

From: Daniel Borkmann
> Sent: 27 May 2015 10:34
...
> >> Fixes: 299ee123e198 (sctp: Fixup v4mapped behaviour to comply with Sock API)
> > ...
> >> This bugfix should be a candidate for -stable
> >
> > Anyone know off-hand which kernel releases are affected?
> > I'm going to have to note this in the release notes for one of our products.
> 
> Ever heard of git-describe(1) ? ;)
> 
> git describe 299ee123e19889d511092347f5fc14db0f10e3a6
> v3.16-rc7-1525-g299ee12

Probably last time I asked the same question :-)
Since I don't spend all day fighting git, I don't know all the sub-commands.

In any case it looks like I can escape by turning off
SCTP_I_WANT_MAPPED_V4_ADDR for kernels 3.17 through 4.0.

	David

Re: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[Jason Gunthorpe]

On Wed, May 27, 2015 at 10:11:22AM +0000, David Laight wrote:

> In any case it looks like I can escape by turning off
> SCTP_I_WANT_MAPPED_V4_ADDR for kernels 3.17 through 4.0.

Just be aware that option is unusable on kernels without 299ee.

I fixed everything wrong I saw, but that doesn't mean it works
100%. Honestly, I don't think anyone has ever used it.

Jason

RE: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[David Laight]

From: Jason Gunthorpe
> Sent: 27 May 2015 16:32
> On Wed, May 27, 2015 at 10:11:22AM +0000, David Laight wrote:
> 
> > In any case it looks like I can escape by turning off
> > SCTP_I_WANT_MAPPED_V4_ADDR for kernels 3.17 through 4.0.
> 
> Just be aware that option is unusable on kernels without 299ee.
> 
> I fixed everything wrong I saw, but that doesn't mean it works
> 100%. Honestly, I don't think anyone has ever used it.

I'm now confused.

I've just done a test using a 4.0.0-rc1 kernel.
I'm binding an IPv6 listening socket and then connecting to it
from 127.0.0.1.
I don't know it I'm being given an IPv4 format address or a
v6mapped one (I shorten the latter before tracing it) - but
it contains 127.0.0.1 (not 0.0.0.0).
(That is without changing any socket options.)

The kernel code I have seems to default 'v4mapped = 1' which means
it should (if I read the code correctly) hit sctp_v4_map_v6().
But I'm not seeing the corruption.
Does 127.0.0.1 go through a different path that means the address
is already in IPv6 format?

Testing without using the loopback address is rather harder (for automated
test scripts).

(If anyone suggests that network namespaces might help, they can then
explain how a single kernel entity is going to choose between different
namespaces on an individual connection basis.
Think of something that is receiving data on one connection, decoding the
requests, re-encapsulation the data in a different protocol and then
sending the data onwards.)

	David

Re: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[Jason Gunthorpe]

On Wed, May 27, 2015 at 04:16:44PM +0000, David Laight wrote:
> From: Jason Gunthorpe
> > Sent: 27 May 2015 16:32
> > On Wed, May 27, 2015 at 10:11:22AM +0000, David Laight wrote:
> > 
> > > In any case it looks like I can escape by turning off
> > > SCTP_I_WANT_MAPPED_V4_ADDR for kernels 3.17 through 4.0.
> > 
> > Just be aware that option is unusable on kernels without 299ee.
> > 
> > I fixed everything wrong I saw, but that doesn't mean it works
> > 100%. Honestly, I don't think anyone has ever used it.
> 
> I'm now confused.
> 
> I've just done a test using a 4.0.0-rc1 kernel.
> I'm binding an IPv6 listening socket and then connecting to it
> from 127.0.0.1.
> I don't know it I'm being given an IPv4 format address or a
> v6mapped one (I shorten the latter before tracing it) - but
> it contains 127.0.0.1 (not 0.0.0.0).
> (That is without changing any socket options.)

I don't know what your test does, but I used the same basic idea with
loopback to find this issue. You should confirm the kernel is
returning a AF_INET6 socket type, if it is AF_INET then there is a
path I missed in 299ee and I should fix it..

Specifically, the corruption I confirmed was from a recvmsg call with
MSG_NOTIFICATION set indicating a new connection has happened on a
many to many socket.

strace sayth:

socket(PF_INET6, SOCK_SEQPACKET|SOCK_CLOEXEC, IPPROTO_SCTP) = 7
recvmsg(7, {msg_name(28)={sa_family=AF_INET6, sin6_port=htons(9090), inet_pton(AF_INET6, "::ffff:0.0.0.0", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, msg_iov(1)=[{"\1\200\0\0\24\0\0\0\4\0\0\0\0\0\0\0\17%\0\0", 1024}], msg_controllen=0, msg_flags=MSG_EOR|MSG_MORE}, MSG_DONTWAIT) = 20

Jason

RE: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[David Laight]

From: Jason Gunthorpe
> Sent: 27 May 2015 17:32
> On Wed, May 27, 2015 at 04:16:44PM +0000, David Laight wrote:
> > From: Jason Gunthorpe
> > > Sent: 27 May 2015 16:32
> > > On Wed, May 27, 2015 at 10:11:22AM +0000, David Laight wrote:
> > >
> > > > In any case it looks like I can escape by turning off
> > > > SCTP_I_WANT_MAPPED_V4_ADDR for kernels 3.17 through 4.0.
> > >
> > > Just be aware that option is unusable on kernels without 299ee.
> > >
> > > I fixed everything wrong I saw, but that doesn't mean it works
> > > 100%. Honestly, I don't think anyone has ever used it.
> >
> > I'm now confused.
> >
> > I've just done a test using a 4.0.0-rc1 kernel.
> > I'm binding an IPv6 listening socket and then connecting to it
> > from 127.0.0.1.
> > I don't know it I'm being given an IPv4 format address or a
> > v6mapped one (I shorten the latter before tracing it) - but
> > it contains 127.0.0.1 (not 0.0.0.0).
> > (That is without changing any socket options.)
> 
> I don't know what your test does, but I used the same basic idea with
> loopback to find this issue. You should confirm the kernel is
> returning a AF_INET6 socket type, if it is AF_INET then there is a
> path I missed in 299ee and I should fix it..

The code will be sleeping in kernel_accept() and later calls
kernel_getpeername().
The code is used for both TCP and SCTP and this part is common (using
the TCP semantics).

I'll look at the actual raw address format tomorrow.
I suspect it is already AF_INET6 before getting to the code that would
badly translate it.

	David

Re: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[Jason Gunthorpe]

On Wed, May 27, 2015 at 04:41:18PM +0000, David Laight wrote:

> The code will be sleeping in kernel_accept() and later calls
> kernel_getpeername().
> The code is used for both TCP and SCTP and this part is common (using
> the TCP semantics).

getpeername uses a different flow, it calls into inet6_getname which
will always return the AF_INET6 version.

The call to sctp_v6_addr_to_user after is to support the v6->v4 coversion
when SCTP_I_WANT_MAPPED_V4_ADDR=0, it will never do the broken v4->v6
conversion.

Jason

RE: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[David Laight]

From: Jason Gunthorpe
> Sent: 27 May 2015 18:05
> On Wed, May 27, 2015 at 04:41:18PM +0000, David Laight wrote:
> 
> > The code will be sleeping in kernel_accept() and later calls
> > kernel_getpeername().
> > The code is used for both TCP and SCTP and this part is common (using
> > the TCP semantics).
> 
> getpeername uses a different flow, it calls into inet6_getname which
> will always return the AF_INET6 version.

Ok, that explains why I hadn't seen the problem.
It also means I don't have to worry about it.

	David

Re: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[Neil Horman]

On Tue, May 26, 2015 at 05:30:17PM -0600, Jason Gunthorpe wrote:
> sctp_v4_map_v6 was subtly writing and reading from members
> of a union in a way the clobbered data it needed to read before
> it read it.
> 
> Zeroing the v6 flowinfo overwrites the v4 sin_addr with 0, meaning
> that every place that calls sctp_v4_map_v6 gets ::ffff:0.0.0.0 as the
> result.
> 
> Reorder things to guarantee correct behaviour no matter what the
> union layout is.
> 
> This impacts user space clients that open an IPv6 SCTP socket and
> receive IPv4 connections. Prior to 299ee user space would see a
> sockaddr with AF_INET and a correct address, after 299ee the sockaddr
> is AF_INET6, but the address is wrong.
> 
> Fixes: 299ee123e198 (sctp: Fixup v4mapped behaviour to comply with Sock API)
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  include/net/sctp/sctp.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> This bugfix should be a candidate for -stable
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 856f01cb51dd..230775f5952a 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -571,11 +571,14 @@ static inline void sctp_v6_map_v4(union sctp_addr *addr)
>  /* Map v4 address to v4-mapped v6 address */
>  static inline void sctp_v4_map_v6(union sctp_addr *addr)
>  {
> +	__be16 port;
> +
> +	port = addr->v4.sin_port;
> +	addr->v6.sin6_addr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
> +	addr->v6.sin6_port = port;
>  	addr->v6.sin6_family = AF_INET6;
>  	addr->v6.sin6_flowinfo = 0;
>  	addr->v6.sin6_scope_id = 0;
> -	addr->v6.sin6_port = addr->v4.sin_port;
> -	addr->v6.sin6_addr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
>  	addr->v6.sin6_addr.s6_addr32[0] = 0;
>  	addr->v6.sin6_addr.s6_addr32[1] = 0;
>  	addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Nice catch
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

[David Miller]

From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Date: Tue, 26 May 2015 17:30:17 -0600

> sctp_v4_map_v6 was subtly writing and reading from members
> of a union in a way the clobbered data it needed to read before
> it read it.
> 
> Zeroing the v6 flowinfo overwrites the v4 sin_addr with 0, meaning
> that every place that calls sctp_v4_map_v6 gets ::ffff:0.0.0.0 as the
> result.
> 
> Reorder things to guarantee correct behaviour no matter what the
> union layout is.
> 
> This impacts user space clients that open an IPv6 SCTP socket and
> receive IPv4 connections. Prior to 299ee user space would see a
> sockaddr with AF_INET and a correct address, after 299ee the sockaddr
> is AF_INET6, but the address is wrong.
> 
> Fixes: 299ee123e198 (sctp: Fixup v4mapped behaviour to comply with Sock API)
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Applied and queued up for -stable, thakns.