[PATCH net v2 0/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[PATCH net v2 0/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[Paolo Abeni]

The syzbot reported an l2tp oops that uncovered some races in the l2tp xmit
path and a partially related issue in the generic ipv6 code.

We need to address them separately.

v1 -> v2:
 - add missing fixes tag in patch 1
 - fix several issues in patch 2

Paolo Abeni (2):
  net: ipv6: keep sk status consistent after datagram connect failure
  l2tp: fix races with ipv4-mapped ipv6 addresses

 net/ipv6/datagram.c  | 21 +++++++++++------
 net/l2tp/l2tp_core.c | 65 ++++++++++++++++++++++++++++++++++++++++------------
 net/l2tp/l2tp_core.h |  3 ---
 3 files changed, 64 insertions(+), 25 deletions(-)

-- 
2.14.3

[PATCH net v2 1/2] net: ipv6: keep sk status consistent after datagram connect failure

[Paolo Abeni]

On unsuccesful ip6_datagram_connect(), if the failure is caused by
ip6_datagram_dst_update(), the sk peer information are cleared, but
the sk->sk_state is preserved.

If the socket was already in an established status, the overall sk
status is inconsistent and fouls later checks in datagram code.

Fix this saving the old peer information and restoring them in
case of failure. This also aligns ipv6 datagram connect() behavior
with ipv4.

v1 -> v2:
 - added missing Fixes tag

Fixes: 85cb73ff9b74 ("net: ipv6: reset daddr and dport in sk if connect() fails")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv6/datagram.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index fbf08ce3f5ab..8a9ac2d0f5d3 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -146,10 +146,12 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
 	struct sockaddr_in6	*usin = (struct sockaddr_in6 *) uaddr;
 	struct inet_sock	*inet = inet_sk(sk);
 	struct ipv6_pinfo	*np = inet6_sk(sk);
-	struct in6_addr		*daddr;
+	struct in6_addr		*daddr, old_daddr;
+	__be32			fl6_flowlabel = 0;
+	__be32			old_fl6_flowlabel;
+	__be32			old_dport;
 	int			addr_type;
 	int			err;
-	__be32			fl6_flowlabel = 0;
 
 	if (usin->sin6_family == AF_INET) {
 		if (__ipv6_only_sock(sk))
@@ -238,9 +240,13 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
 		}
 	}
 
+	/* save the current peer information before updating it */
+	old_daddr = sk->sk_v6_daddr;
+	old_fl6_flowlabel = np->flow_label;
+	old_dport = inet->inet_dport;
+
 	sk->sk_v6_daddr = *daddr;
 	np->flow_label = fl6_flowlabel;
-
 	inet->inet_dport = usin->sin6_port;
 
 	/*
@@ -250,11 +256,12 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
 
 	err = ip6_datagram_dst_update(sk, true);
 	if (err) {
-		/* Reset daddr and dport so that udp_v6_early_demux()
-		 * fails to find this socket
+		/* Restore the socket peer info, to keep it consistent with
+		 * the old socket state
 		 */
-		memset(&sk->sk_v6_daddr, 0, sizeof(sk->sk_v6_daddr));
-		inet->inet_dport = 0;
+		sk->sk_v6_daddr = old_daddr;
+		np->flow_label = old_fl6_flowlabel;
+		inet->inet_dport = old_dport;
 		goto out;
 	}
 
-- 
2.14.3

[PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[Paolo Abeni]

When creating a new socket, l2tp_tunnel_create() ensures that
such socket is connected, but when using a socket provided by
the user space, no check is done on the socket state.

This may foul the later check for ipv6 sockets that are
ipv4-mapped, e.g. in case of unconnected ipv6 socket bound to
ipv4 address.

Moreover the connection status and/or peer of a user-space
controlled socket may change at runtime.

This change addresses the issues:
* explicitly checking for TCP_ESTABLISHED for user space provided sockets
* dropping the v4mapped flag usage - it can become outdated - and
  explicitly invoking ipv6_addr_v4mapped() instead
* refreshing the inet_sk copy of ipv4-mapped ipv6 address at xmit time.

The issue is apparently there since ancient times.

v1 -> v2: (many thanks to Guillaume)
 - with csum issue introduced in v1
 - replace pr_err with pr_debug
 - fix build issue with IPV6 disabled
 - move l2tp_sk_is_v4mapped in l2tp_core.c

Reported-and-tested-by: syzbot+92fa328176eb07e4ac1a@syzkaller.appspotmail.com
Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/l2tp/l2tp_core.c | 65 ++++++++++++++++++++++++++++++++++++++++------------
 net/l2tp/l2tp_core.h |  3 ---
 2 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 83421c6f0bef..9726e3f37745 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -111,6 +111,19 @@ struct l2tp_net {
 	spinlock_t l2tp_session_hlist_lock;
 };
 
+#if IS_ENABLED(CONFIG_IPV6)
+static bool l2tp_sk_is_v4mapped(struct sock *sk)
+{
+	return sk->sk_family == PF_INET6 &&
+	       ipv6_addr_v4mapped(&sk->sk_v6_daddr);
+}
+
+static bool l2tp_sk_is_v6(struct sock *sk)
+{
+	return sk->sk_family == PF_INET6 &&
+	       !ipv6_addr_v4mapped(&sk->sk_v6_daddr);
+}
+#endif
 
 static inline struct l2tp_tunnel *l2tp_tunnel(struct sock *sk)
 {
@@ -1049,7 +1062,7 @@ static int l2tp_xmit_core(struct l2tp_session *session, struct sk_buff *skb,
 	/* Queue the packet to IP for output */
 	skb->ignore_df = 1;
 #if IS_ENABLED(CONFIG_IPV6)
-	if (tunnel->sock->sk_family == PF_INET6 && !tunnel->v4mapped)
+	if (l2tp_sk_is_v6(tunnel->sock))
 		error = inet6_csk_xmit(tunnel->sock, skb, NULL);
 	else
 #endif
@@ -1112,11 +1125,32 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
 		goto out_unlock;
 	}
 
+	/* The user-space may change the connection status for the user-space
+	 * provided socket at run time: we must check it under the socket lock
+	 */
+	inet = inet_sk(sk);
+	if (tunnel->fd >= 0) {
+		if (sk->sk_state != TCP_ESTABLISHED) {
+			ret = NET_XMIT_DROP;
+			goto out_unlock;
+		}
+
+#if IS_ENABLED(CONFIG_IPV6)
+		/* If the uses space changes the ipv4-mapped ipv6 address,
+		 * the kernel copy of the ipv4 address is not updated.
+		 * Refresh it only if needed, to avoid dirtying the socket
+		 * on each packet.
+		 */
+		if (l2tp_sk_is_v4mapped(sk) &&
+		    inet->inet_daddr != sk->sk_v6_daddr.s6_addr32[3])
+			inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
+#endif
+	}
+
 	/* Get routing info from the tunnel socket */
 	skb_dst_drop(skb);
 	skb_dst_set(skb, dst_clone(__sk_dst_check(sk, 0)));
 
-	inet = inet_sk(sk);
 	fl = &inet->cork.fl;
 	switch (tunnel->encap) {
 	case L2TP_ENCAPTYPE_UDP:
@@ -1131,7 +1165,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
 
 		/* Calculate UDP checksum if configured to do so */
 #if IS_ENABLED(CONFIG_IPV6)
-		if (sk->sk_family == PF_INET6 && !tunnel->v4mapped)
+		if (l2tp_sk_is_v6(sk))
 			udp6_set_csum(udp_get_no_check6_tx(sk),
 				      skb, &inet6_sk(sk)->saddr,
 				      &sk->sk_v6_daddr, udp_len);
@@ -1449,6 +1483,14 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 			err = -EINVAL;
 			goto err;
 		}
+
+		/* Reject unconnected sockets */
+		if (sock->sk->sk_state != TCP_ESTABLISHED) {
+			pr_debug("tunl %u: sock fd=%d is unconnected\n",
+			       tunnel_id, fd);
+			err = -EINVAL;
+			goto err;
+		}
 	}
 
 	sk = sock->sk;
@@ -1508,20 +1550,13 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 		tunnel->debug = cfg->debug;
 
 #if IS_ENABLED(CONFIG_IPV6)
-	if (sk->sk_family == PF_INET6) {
+	if (l2tp_sk_is_v4mapped(sk)) {
 		struct ipv6_pinfo *np = inet6_sk(sk);
+		struct inet_sock *inet = inet_sk(sk);
 
-		if (ipv6_addr_v4mapped(&np->saddr) &&
-		    ipv6_addr_v4mapped(&sk->sk_v6_daddr)) {
-			struct inet_sock *inet = inet_sk(sk);
-
-			tunnel->v4mapped = true;
-			inet->inet_saddr = np->saddr.s6_addr32[3];
-			inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
-			inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
-		} else {
-			tunnel->v4mapped = false;
-		}
+		inet->inet_saddr = np->saddr.s6_addr32[3];
+		inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
+		inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
 	}
 #endif
 
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index a1aa9550f04e..2718d0b284d0 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -188,9 +188,6 @@ struct l2tp_tunnel {
 	struct sock		*sock;		/* Parent socket */
 	int			fd;		/* Parent fd, if tunnel socket
 						 * was created by userspace */
-#if IS_ENABLED(CONFIG_IPV6)
-	bool			v4mapped;
-#endif
 
 	struct work_struct	del_work;
 
-- 
2.14.3

Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[Guillaume Nault]

> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 83421c6f0bef..9726e3f37745 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1112,11 +1125,32 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
>  		goto out_unlock;
>  	}
>  
> +	/* The user-space may change the connection status for the user-space
> +	 * provided socket at run time: we must check it under the socket lock
> +	 */
> +	inet = inet_sk(sk);
> +	if (tunnel->fd >= 0) {
> +		if (sk->sk_state != TCP_ESTABLISHED) {
> +			ret = NET_XMIT_DROP;
> +			goto out_unlock;
> +		}
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +		/* If the uses space changes the ipv4-mapped ipv6 address,
I guess you meant 'user-space'.

> +		 * the kernel copy of the ipv4 address is not updated.
> +		 * Refresh it only if needed, to avoid dirtying the socket
> +		 * on each packet.
> +		 */
> +		if (l2tp_sk_is_v4mapped(sk) &&
> +		    inet->inet_daddr != sk->sk_v6_daddr.s6_addr32[3])
I can't see how to trigger this condition. Re-connecting the socket
doesn't do, as __ip6_datagram_connect() already updates both
->inet_daddr and ->sk_v6_daddr.

> @@ -1131,7 +1165,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
>  
>  		/* Calculate UDP checksum if configured to do so */
>  #if IS_ENABLED(CONFIG_IPV6)
> -		if (sk->sk_family == PF_INET6 && !tunnel->v4mapped)
> +		if (l2tp_sk_is_v6(sk))
>  			udp6_set_csum(udp_get_no_check6_tx(sk),
>  				      skb, &inet6_sk(sk)->saddr,
>  				      &sk->sk_v6_daddr, udp_len);
> @@ -1449,6 +1483,14 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
>  			err = -EINVAL;
>  			goto err;
>  		}
> +
> +		/* Reject unconnected sockets */
> +		if (sock->sk->sk_state != TCP_ESTABLISHED) {
> +			pr_debug("tunl %u: sock fd=%d is unconnected\n",
> +			       tunnel_id, fd);
> +			err = -EINVAL;
-ENOTCONN looks more appropriate. And BTW, the socket isn't locked here.

> @@ -1508,20 +1550,13 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
>  		tunnel->debug = cfg->debug;
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> -	if (sk->sk_family == PF_INET6) {
> +	if (l2tp_sk_is_v4mapped(sk)) {
>  		struct ipv6_pinfo *np = inet6_sk(sk);
> +		struct inet_sock *inet = inet_sk(sk);
>  
> -		if (ipv6_addr_v4mapped(&np->saddr) &&
> -		    ipv6_addr_v4mapped(&sk->sk_v6_daddr)) {
> -			struct inet_sock *inet = inet_sk(sk);
> -
> -			tunnel->v4mapped = true;
> -			inet->inet_saddr = np->saddr.s6_addr32[3];
> -			inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> -			inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> -		} else {
> -			tunnel->v4mapped = false;
> -		}
> +		inet->inet_saddr = np->saddr.s6_addr32[3];
> +		inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> +		inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
>  	}
Same question as for the l2tp_xmit_skb() part: how can one create a
v4mapped socket with unset (or invalid) ->inet_*addr?
I don't have much experience with v4mapped sockets, but I couldn't get
a scenario where inet->inet_*addr weren't properly set already.

Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[Paolo Abeni]

Hi,

On Fri, 2018-03-09 at 17:43 +0100, Guillaume Nault wrote:
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > index 83421c6f0bef..9726e3f37745 100644
> > --- a/net/l2tp/l2tp_core.c
> > +++ b/net/l2tp/l2tp_core.c
> > @@ -1112,11 +1125,32 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
> >  		goto out_unlock;
> >  	}
> >  
> > +	/* The user-space may change the connection status for the user-space
> > +	 * provided socket at run time: we must check it under the socket lock
> > +	 */
> > +	inet = inet_sk(sk);
> > +	if (tunnel->fd >= 0) {
> > +		if (sk->sk_state != TCP_ESTABLISHED) {
> > +			ret = NET_XMIT_DROP;
> > +			goto out_unlock;
> > +		}
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +		/* If the uses space changes the ipv4-mapped ipv6 address,
> 
> I guess you meant 'user-space'.

yep ;)

> > +		 * the kernel copy of the ipv4 address is not updated.
> > +		 * Refresh it only if needed, to avoid dirtying the socket
> > +		 * on each packet.
> > +		 */
> > +		if (l2tp_sk_is_v4mapped(sk) &&
> > +		    inet->inet_daddr != sk->sk_v6_daddr.s6_addr32[3])
> 
> I can't see how to trigger this condition. Re-connecting the socket
> doesn't do, as __ip6_datagram_connect() already updates both
> ->inet_daddr and ->sk_v6_daddr.

Uh, apparently I trusted too much the original code below...

> > @@ -1131,7 +1165,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
> >  
> >  		/* Calculate UDP checksum if configured to do so */
> >  #if IS_ENABLED(CONFIG_IPV6)
> > -		if (sk->sk_family == PF_INET6 && !tunnel->v4mapped)
> > +		if (l2tp_sk_is_v6(sk))
> >  			udp6_set_csum(udp_get_no_check6_tx(sk),
> >  				      skb, &inet6_sk(sk)->saddr,
> >  				      &sk->sk_v6_daddr, udp_len);
> > @@ -1449,6 +1483,14 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> >  			err = -EINVAL;
> >  			goto err;
> >  		}
> > +
> > +		/* Reject unconnected sockets */
> > +		if (sock->sk->sk_state != TCP_ESTABLISHED) {
> > +			pr_debug("tunl %u: sock fd=%d is unconnected\n",
> > +			       tunnel_id, fd);
> > +			err = -EINVAL;
> 
> -ENOTCONN looks more appropriate. And BTW, the socket isn't locked here.

Ok. I think locking is not needed if we only check the sk state.
> 
> > @@ -1508,20 +1550,13 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> >  		tunnel->debug = cfg->debug;
> >  
> >  #if IS_ENABLED(CONFIG_IPV6)
> > -	if (sk->sk_family == PF_INET6) {
> > +	if (l2tp_sk_is_v4mapped(sk)) {
> >  		struct ipv6_pinfo *np = inet6_sk(sk);
> > +		struct inet_sock *inet = inet_sk(sk);
> >  
> > -		if (ipv6_addr_v4mapped(&np->saddr) &&
> > -		    ipv6_addr_v4mapped(&sk->sk_v6_daddr)) {
> > -			struct inet_sock *inet = inet_sk(sk);
> > -
> > -			tunnel->v4mapped = true;
> > -			inet->inet_saddr = np->saddr.s6_addr32[3];
> > -			inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > -			inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> > -		} else {
> > -			tunnel->v4mapped = false;
> > -		}
> > +		inet->inet_saddr = np->saddr.s6_addr32[3];
> > +		inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > +		inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> >  	}
> 
> Same question as for the l2tp_xmit_skb() part: how can one create a
> v4mapped socket with unset (or invalid) ->inet_*addr?

Double-checking the ipv6 connect, apparently we don't need to copy the
ipv4 mapped address, so the above chunk could be dropped.

The syzbot reproducer is able to trigger the condition you describe
calling connect() 2 consecutive times, the first one on an ipv4 mapped
address and the second one an (unmapped) ipv6 address.

I will send a v3.

Thanks,

Paolo

Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[Guillaume Nault]

On Fri, Mar 09, 2018 at 06:04:03PM +0100, Paolo Abeni wrote:
> Hi,
> 
> On Fri, 2018-03-09 at 17:43 +0100, Guillaume Nault wrote:
> > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > > index 83421c6f0bef..9726e3f37745 100644
> > > --- a/net/l2tp/l2tp_core.c
> > > +++ b/net/l2tp/l2tp_core.c
> > > @@ -1131,7 +1165,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
> > >  
> > >  		/* Calculate UDP checksum if configured to do so */
> > >  #if IS_ENABLED(CONFIG_IPV6)
> > > -		if (sk->sk_family == PF_INET6 && !tunnel->v4mapped)
> > > +		if (l2tp_sk_is_v6(sk))
> > >  			udp6_set_csum(udp_get_no_check6_tx(sk),
> > >  				      skb, &inet6_sk(sk)->saddr,
> > >  				      &sk->sk_v6_daddr, udp_len);
> > > @@ -1449,6 +1483,14 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> > >  			err = -EINVAL;
> > >  			goto err;
> > >  		}
> > > +
> > > +		/* Reject unconnected sockets */
> > > +		if (sock->sk->sk_state != TCP_ESTABLISHED) {
> > > +			pr_debug("tunl %u: sock fd=%d is unconnected\n",
> > > +			       tunnel_id, fd);
> > > +			err = -EINVAL;
> > 
> > -ENOTCONN looks more appropriate. And BTW, the socket isn't locked here.
> 
> Ok. I think locking is not needed if we only check the sk state.
> 
Yes, the race is harmless, sure, but that makes the test much less
valuable. It tells reviewers and tools like syzbot, that only connected
sockets can be used. While, in reality, the race allows bypassing the
test.

> > > @@ -1508,20 +1550,13 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> > >  		tunnel->debug = cfg->debug;
> > >  
> > >  #if IS_ENABLED(CONFIG_IPV6)
> > > -	if (sk->sk_family == PF_INET6) {
> > > +	if (l2tp_sk_is_v4mapped(sk)) {
> > >  		struct ipv6_pinfo *np = inet6_sk(sk);
> > > +		struct inet_sock *inet = inet_sk(sk);
> > >  
> > > -		if (ipv6_addr_v4mapped(&np->saddr) &&
> > > -		    ipv6_addr_v4mapped(&sk->sk_v6_daddr)) {
> > > -			struct inet_sock *inet = inet_sk(sk);
> > > -
> > > -			tunnel->v4mapped = true;
> > > -			inet->inet_saddr = np->saddr.s6_addr32[3];
> > > -			inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > > -			inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> > > -		} else {
> > > -			tunnel->v4mapped = false;
> > > -		}
> > > +		inet->inet_saddr = np->saddr.s6_addr32[3];
> > > +		inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > > +		inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> > >  	}
> > 
> > Same question as for the l2tp_xmit_skb() part: how can one create a
> > v4mapped socket with unset (or invalid) ->inet_*addr?
> 
> Double-checking the ipv6 connect, apparently we don't need to copy the
> ipv4 mapped address, so the above chunk could be dropped.
> 
> The syzbot reproducer is able to trigger the condition you describe
> calling connect() 2 consecutive times, the first one on an ipv4 mapped
> address and the second one an (unmapped) ipv6 address.
> 
Yes, but that's before your patch 1/2. The reproducer doesn't seem to
work anymore once it's applied.

Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[Paolo Abeni]

On Fri, 2018-03-09 at 18:47 +0100, Guillaume Nault wrote:
> On Fri, Mar 09, 2018 at 06:04:03PM +0100, Paolo Abeni wrote:
> > Hi,
> > 
> > On Fri, 2018-03-09 at 17:43 +0100, Guillaume Nault wrote:
> > > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > > > index 83421c6f0bef..9726e3f37745 100644
> > > > --- a/net/l2tp/l2tp_core.c
> > > > +++ b/net/l2tp/l2tp_core.c
> > > > @@ -1131,7 +1165,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
> > > >  
> > > >  		/* Calculate UDP checksum if configured to do so */
> > > >  #if IS_ENABLED(CONFIG_IPV6)
> > > > -		if (sk->sk_family == PF_INET6 && !tunnel->v4mapped)
> > > > +		if (l2tp_sk_is_v6(sk))
> > > >  			udp6_set_csum(udp_get_no_check6_tx(sk),
> > > >  				      skb, &inet6_sk(sk)->saddr,
> > > >  				      &sk->sk_v6_daddr, udp_len);
> > > > @@ -1449,6 +1483,14 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> > > >  			err = -EINVAL;
> > > >  			goto err;
> > > >  		}
> > > > +
> > > > +		/* Reject unconnected sockets */
> > > > +		if (sock->sk->sk_state != TCP_ESTABLISHED) {
> > > > +			pr_debug("tunl %u: sock fd=%d is unconnected\n",
> > > > +			       tunnel_id, fd);
> > > > +			err = -EINVAL;
> > > 
> > > -ENOTCONN looks more appropriate. And BTW, the socket isn't locked here.
> > 
> > Ok. I think locking is not needed if we only check the sk state.
> > 
> 
> Yes, the race is harmless, sure, but that makes the test much less
> valuable. It tells reviewers and tools like syzbot, that only connected
> sockets can be used. While, in reality, the race allows bypassing the
> test.

I'll drop the above in v3

> > > > @@ -1508,20 +1550,13 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> > > >  		tunnel->debug = cfg->debug;
> > > >  
> > > >  #if IS_ENABLED(CONFIG_IPV6)
> > > > -	if (sk->sk_family == PF_INET6) {
> > > > +	if (l2tp_sk_is_v4mapped(sk)) {
> > > >  		struct ipv6_pinfo *np = inet6_sk(sk);
> > > > +		struct inet_sock *inet = inet_sk(sk);
> > > >  
> > > > -		if (ipv6_addr_v4mapped(&np->saddr) &&
> > > > -		    ipv6_addr_v4mapped(&sk->sk_v6_daddr)) {
> > > > -			struct inet_sock *inet = inet_sk(sk);
> > > > -
> > > > -			tunnel->v4mapped = true;
> > > > -			inet->inet_saddr = np->saddr.s6_addr32[3];
> > > > -			inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > > > -			inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> > > > -		} else {
> > > > -			tunnel->v4mapped = false;
> > > > -		}
> > > > +		inet->inet_saddr = np->saddr.s6_addr32[3];
> > > > +		inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > > > +		inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> > > >  	}
> > > 
> > > Same question as for the l2tp_xmit_skb() part: how can one create a
> > > v4mapped socket with unset (or invalid) ->inet_*addr?
> > 
> > Double-checking the ipv6 connect, apparently we don't need to copy the
> > ipv4 mapped address, so the above chunk could be dropped.
> > 
> > The syzbot reproducer is able to trigger the condition you describe
> > calling connect() 2 consecutive times, the first one on an ipv4 mapped
> > address and the second one an (unmapped) ipv6 address.
> > 
> 
> Yes, but that's before your patch 1/2. The reproducer doesn't seem to
> work anymore once it's applied.

The single threaded reproducer does not trigger anymore after 1/2,
_but_ if ask syzbot to test 1/2 that will trigger another splat,
because syzbot will do also multi threaded tests and we will hit the
race between connect(tunnel->fd) and l2tp_tunnel_create(), please have
a look at:

https://groups.google.com/forum/#!topic/syzkaller-bugs/N53POqzMUa0

Cheers,

Paolo

Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[Guillaume Nault]

On Fri, Mar 09, 2018 at 06:58:00PM +0100, Paolo Abeni wrote:
> On Fri, 2018-03-09 at 18:47 +0100, Guillaume Nault wrote:
> > On Fri, Mar 09, 2018 at 06:04:03PM +0100, Paolo Abeni wrote:
> > > Hi,
> > > 
> > > On Fri, 2018-03-09 at 17:43 +0100, Guillaume Nault wrote:
> > > > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > > > > index 83421c6f0bef..9726e3f37745 100644
> > > > > --- a/net/l2tp/l2tp_core.c
> > > > > +++ b/net/l2tp/l2tp_core.c
> > > > > @@ -1131,7 +1165,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
> > > > >  
> > > > >  		/* Calculate UDP checksum if configured to do so */
> > > > >  #if IS_ENABLED(CONFIG_IPV6)
> > > > > -		if (sk->sk_family == PF_INET6 && !tunnel->v4mapped)
> > > > > +		if (l2tp_sk_is_v6(sk))
> > > > >  			udp6_set_csum(udp_get_no_check6_tx(sk),
> > > > >  				      skb, &inet6_sk(sk)->saddr,
> > > > >  				      &sk->sk_v6_daddr, udp_len);
> > > > > @@ -1449,6 +1483,14 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> > > > >  			err = -EINVAL;
> > > > >  			goto err;
> > > > >  		}
> > > > > +
> > > > > +		/* Reject unconnected sockets */
> > > > > +		if (sock->sk->sk_state != TCP_ESTABLISHED) {
> > > > > +			pr_debug("tunl %u: sock fd=%d is unconnected\n",
> > > > > +			       tunnel_id, fd);
> > > > > +			err = -EINVAL;
> > > > 
> > > > -ENOTCONN looks more appropriate. And BTW, the socket isn't locked here.
> > > 
> > > Ok. I think locking is not needed if we only check the sk state.
> > > 
> > 
> > Yes, the race is harmless, sure, but that makes the test much less
> > valuable. It tells reviewers and tools like syzbot, that only connected
> > sockets can be used. While, in reality, the race allows bypassing the
> > test.
> 
> I'll drop the above in v3
> 
> > > > > @@ -1508,20 +1550,13 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> > > > >  		tunnel->debug = cfg->debug;
> > > > >  
> > > > >  #if IS_ENABLED(CONFIG_IPV6)
> > > > > -	if (sk->sk_family == PF_INET6) {
> > > > > +	if (l2tp_sk_is_v4mapped(sk)) {
> > > > >  		struct ipv6_pinfo *np = inet6_sk(sk);
> > > > > +		struct inet_sock *inet = inet_sk(sk);
> > > > >  
> > > > > -		if (ipv6_addr_v4mapped(&np->saddr) &&
> > > > > -		    ipv6_addr_v4mapped(&sk->sk_v6_daddr)) {
> > > > > -			struct inet_sock *inet = inet_sk(sk);
> > > > > -
> > > > > -			tunnel->v4mapped = true;
> > > > > -			inet->inet_saddr = np->saddr.s6_addr32[3];
> > > > > -			inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > > > > -			inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> > > > > -		} else {
> > > > > -			tunnel->v4mapped = false;
> > > > > -		}
> > > > > +		inet->inet_saddr = np->saddr.s6_addr32[3];
> > > > > +		inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > > > > +		inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> > > > >  	}
> > > > 
> > > > Same question as for the l2tp_xmit_skb() part: how can one create a
> > > > v4mapped socket with unset (or invalid) ->inet_*addr?
> > > 
> > > Double-checking the ipv6 connect, apparently we don't need to copy the
> > > ipv4 mapped address, so the above chunk could be dropped.
> > > 
> > > The syzbot reproducer is able to trigger the condition you describe
> > > calling connect() 2 consecutive times, the first one on an ipv4 mapped
> > > address and the second one an (unmapped) ipv6 address.
> > > 
> > 
> > Yes, but that's before your patch 1/2. The reproducer doesn't seem to
> > work anymore once it's applied.
> 
> The single threaded reproducer does not trigger anymore after 1/2,
> _but_ if ask syzbot to test 1/2 that will trigger another splat,
> because syzbot will do also multi threaded tests and we will hit the
> race between connect(tunnel->fd) and l2tp_tunnel_create(),
> 
Ok, and this case is handled by the sk_state test in l2tp_xmit_skb(),
right?
I just want to be sure that I didn't miss anything and that patch 1/2
combined with the socket state check in l2tp_xmit_skb() are enough to
fix the bug. And that overriding ->inet_*addr can be removed entirely
(and safely!).

> please have a look at:
> 
> https://groups.google.com/forum/#!topic/syzkaller-bugs/N53POqzMUa0
> 
I will, thanks for the link.

Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[Paolo Abeni]

On Fri, 2018-03-09 at 19:26 +0100, Guillaume Nault wrote:
> On Fri, Mar 09, 2018 at 06:58:00PM +0100, Paolo Abeni wrote:
> > The single threaded reproducer does not trigger anymore after 1/2,
> > _but_ if ask syzbot to test 1/2 that will trigger another splat,
> > because syzbot will do also multi threaded tests and we will hit the
> > race between connect(tunnel->fd) and l2tp_tunnel_create(),
> > 
> 
> Ok, and this case is handled by the sk_state test in l2tp_xmit_skb(),
> right?

We need both such test and checking for v4mapped address in
l2tp_xmit_skb()

> I just want to be sure that I didn't miss anything and that patch 1/2
> combined with the socket state check in l2tp_xmit_skb() are enough to
> fix the bug. And that overriding ->inet_*addr can be removed entirely
> (and safely!).

I tested the above in vs the repro and in some real use case, but any
additinal pair of eyes are welcome!

I'll send v3 soon.

Cheers,

Paolo

Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses

[Guillaume Nault]

On Mon, Mar 12, 2018 at 09:53:18AM +0100, Paolo Abeni wrote:
> On Fri, 2018-03-09 at 19:26 +0100, Guillaume Nault wrote:
> > On Fri, Mar 09, 2018 at 06:58:00PM +0100, Paolo Abeni wrote:
> > > The single threaded reproducer does not trigger anymore after 1/2,
> > > _but_ if ask syzbot to test 1/2 that will trigger another splat,
> > > because syzbot will do also multi threaded tests and we will hit the
> > > race between connect(tunnel->fd) and l2tp_tunnel_create(),
> > > 
> > 
> > Ok, and this case is handled by the sk_state test in l2tp_xmit_skb(),
> > right?
> 
> We need both such test and checking for v4mapped address in
> l2tp_xmit_skb()
> 
> > I just want to be sure that I didn't miss anything and that patch 1/2
> > combined with the socket state check in l2tp_xmit_skb() are enough to
> > fix the bug. And that overriding ->inet_*addr can be removed entirely
> > (and safely!).
> 
> I tested the above in vs the repro and in some real use case, but any
> additinal pair of eyes are welcome!
> 
It looks good to me too. I think everything is cleared up now.
Nice work!