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

[PATCH net 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.

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 | 58 +++++++++++++++++++++++++++++++++-------------------
 net/l2tp/l2tp_core.h | 13 +++++++++---
 3 files changed, 61 insertions(+), 31 deletions(-)

-- 
2.14.3

[PATCH net 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.

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

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 | 58 +++++++++++++++++++++++++++++++++-------------------
 net/l2tp/l2tp_core.h | 13 +++++++++---
 2 files changed, 47 insertions(+), 24 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 83421c6f0bef..ad6aa9b64415 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1049,7 +1049,8 @@ 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 (tunnel->sock->sk_family == PF_INET6 &&
+	    !ipv6_addr_v4mapped(&tunnel->sock->sk_v6_daddr))
 		error = inet6_csk_xmit(tunnel->sock, skb, NULL);
 	else
 #endif
@@ -1112,11 +1113,30 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
 		goto out_unlock;
 	}
 
+	/* 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 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];
+	}
+
 	/* 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:
@@ -1130,15 +1150,13 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
 		uh->len = htons(udp_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_v4mapped(sk))
 			udp6_set_csum(udp_get_no_check6_tx(sk),
 				      skb, &inet6_sk(sk)->saddr,
 				      &sk->sk_v6_daddr, udp_len);
 		else
-#endif
-		udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
-			     inet->inet_daddr, udp_len);
+			udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
+				     inet->inet_daddr, udp_len);
 		break;
 
 	case L2TP_ENCAPTYPE_IP:
@@ -1449,6 +1467,13 @@ 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_err("tunl %u: sock fd=%d is unconnected\n",
+			       tunnel_id, fd);
+			goto err;
+		}
 	}
 
 	sk = sock->sk;
@@ -1507,23 +1532,14 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	if (cfg != NULL)
 		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
 
 	/* Mark socket as an encapsulation socket. See net/ipv4/udp.c */
 	tunnel->encap = encap;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index a1aa9550f04e..c042aaeb074b 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;
 
@@ -214,6 +211,16 @@ static inline void *l2tp_session_priv(struct l2tp_session *session)
 	return &session->priv[0];
 }
 
+static bool l2tp_sk_is_v4mapped(struct sock *sk)
+{
+#if IS_ENABLED(CONFIG_IPV6)
+	return sk->sk_family == PF_INET6 &&
+	       ipv6_addr_v4mapped(&sk->sk_v6_daddr);
+#else
+	return 0;
+#endif
+}
+
 struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id);
 void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);
 
-- 
2.14.3

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

[Guillaume Nault]

On Thu, Mar 08, 2018 at 03:37:27PM +0100, Paolo Abeni wrote:
> 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.
> 
Thanks, I started working on this report, but you've been faster :)

Here's a quick review, I haven't had time to test yet.

> @@ -1130,15 +1150,13 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
>  		uh->len = htons(udp_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_v4mapped(sk))
So we're going to call udp6_set_csum() only for v4mapped sockets and
udp_set_csum() for all others (including plain AF_INET6 sockets)?
I that really the intent, or have I missed something?

>  			udp6_set_csum(udp_get_no_check6_tx(sk),
>  				      skb, &inet6_sk(sk)->saddr,
>  				      &sk->sk_v6_daddr, udp_len);
>  		else
> -#endif
> -		udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
> -			     inet->inet_daddr, udp_len);
> +			udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
> +				     inet->inet_daddr, udp_len);
>  		break;
>  
>  	case L2TP_ENCAPTYPE_IP:
> @@ -1449,6 +1467,13 @@ 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_err("tunl %u: sock fd=%d is unconnected\n",
> +			       tunnel_id, fd);
Following Eric's comment in 17cfe79a65f9 ("l2tp: do not accept arbitrary sockets"),
pr_err() should be avoided here.

Also, 'err' needs to be set before jumping.

> +			goto err;
> +		}
>  	}
>  

> diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
> index a1aa9550f04e..c042aaeb074b 100644
> --- a/net/l2tp/l2tp_core.h
> +++ b/net/l2tp/l2tp_core.h
> @@ -214,6 +211,16 @@ static inline void *l2tp_session_priv(struct l2tp_session *session)
>  	return &session->priv[0];
>  }
>  
> +static bool l2tp_sk_is_v4mapped(struct sock *sk)
The 'inline' keyword is missing, making gcc complain about unused
function. But since this function is only used in l2tp_core.c, maybe we
could just move it there.

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

[Paolo Abeni]

Hi,

On Thu, 2018-03-08 at 19:59 +0100, Guillaume Nault wrote:
> On Thu, Mar 08, 2018 at 03:37:27PM +0100, Paolo Abeni wrote:
> > 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.
> > 
> 
> Thanks, I started working on this report, but you've been faster :)
> 
> Here's a quick review, I haven't had time to test yet.
> 
> > @@ -1130,15 +1150,13 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
> >  		uh->len = htons(udp_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_v4mapped(sk))
> 
> So we're going to call udp6_set_csum() only for v4mapped sockets and
> udp_set_csum() for all others (including plain AF_INET6 sockets)?
> I that really the intent, or have I missed something?

It's a bug, thanks for spotting it!

> >  			udp6_set_csum(udp_get_no_check6_tx(sk),
> >  				      skb, &inet6_sk(sk)->saddr,
> >  				      &sk->sk_v6_daddr, udp_len);
> >  		else
> > -#endif
> > -		udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
> > -			     inet->inet_daddr, udp_len);
> > +			udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
> > +				     inet->inet_daddr, udp_len);
> >  		break;
> >  
> >  	case L2TP_ENCAPTYPE_IP:
> > @@ -1449,6 +1467,13 @@ 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_err("tunl %u: sock fd=%d is unconnected\n",
> > +			       tunnel_id, fd);
> 
> Following Eric's comment in 17cfe79a65f9 ("l2tp: do not accept arbitrary sockets"),
> pr_err() should be avoided here.
> 
> Also, 'err' needs to be set before jumping.

Right, thanks.

> > +			goto err;
> > +		}
> >  	}
> >  
> > diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
> > index a1aa9550f04e..c042aaeb074b 100644
> > --- a/net/l2tp/l2tp_core.h
> > +++ b/net/l2tp/l2tp_core.h
> > @@ -214,6 +211,16 @@ static inline void *l2tp_session_priv(struct l2tp_session *session)
> >  	return &session->priv[0];
> >  }
> >  
> > +static bool l2tp_sk_is_v4mapped(struct sock *sk)
> 
> The 'inline' keyword is missing, making gcc complain about unused
> function. But since this function is only used in l2tp_core.c, maybe we
> could just move it there.

Agreed.

I will cover the above in v2.

Thank you for the review.

Paolo