| Message ID | 20150526233017.GB22391@obsidianresearch.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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