l2tp: fix races with ipv4-mapped ipv6 addresses

PATCH Superseded Paolo Abeni 2 participants 10 messages Mar 9, 2018 – Mar 12, 2018

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Paolo Abeni

author maintainer

2018-03-09 09:34 UTC #0 lore raw

v2 cover l2tp: fix races with ipv4-mapped ipv6 addresses

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(-)

Signature · 1 line

2.14.3

Paolo Abeni

author maintainer

2018-03-09 09:34 UTC #1 lore raw

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

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 <[email protected]>

---
 net/ipv6/datagram.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

net/ipv6/datagram.c +14 -7

index fbf08ce3f5ab..8a9ac2d0f5d3 100644
@@ -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;
 	}

Signature · 1 line

2.14.3

Paolo Abeni

author maintainer

2018-03-09 09:34 UTC #2 lore raw

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

In reply to 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 [email protected]

Fixes 3557baabf280 ("[L2TP]: PPP over L2TP driver core")

Signed-off-by Paolo Abeni <[email protected]>

---
 net/l2tp/l2tp_core.c | 65 ++++++++++++++++++++++++++++++++++++++++------------
 net/l2tp/l2tp_core.h |  3 ---
 2 files changed, 50 insertions(+), 18 deletions(-)

net/l2tp/l2tp_core.c +50 ~~-15~~

index 83421c6f0bef..9726e3f37745 100644
@@ -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

net/l2tp/l2tp_core.h +0 -3

index a1aa9550f04e..2718d0b284d0 100644
@@ -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;

Signature · 1 line

2.14.3

Guillaume Nault

2018-03-09 16:43 UTC #3 lore raw

Quoted 10 lines

net/l2tp/l2tp_core.c +2 -0

index 83421c6f0bef..9726e3f37745 100644
@@ -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

net/l2tp/l2tp_core.c int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len · line 1139

Quoted 10 lines

net/l2tp/l2tp_core.c +10 -0

+	 */
+	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'.

net/l2tp/l2tp_core.c int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len · line 1145

Quoted 6 lines

net/l2tp/l2tp_core.c +6 -0

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

Quoted 10 lines

net/l2tp/l2tp_core.c +1 -1

@@ -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);

net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1491

Quoted 9 lines

net/l2tp/l2tp_core.c +6 -0

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

Quoted 14 lines

net/l2tp/l2tp_core.c +2 -6

@@ -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;

net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1560

Quoted 10 lines

net/l2tp/l2tp_core.c +3 -6

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

Paolo Abeni

author maintainer

2018-03-09 17:04 UTC #4 lore raw

Hi,

Quoted 8 lines · Guillaume Nault

Quoted 8 lines

net/l2tp/l2tp_core.c +0 -0

index 83421c6f0bef..9726e3f37745 100644
@@ -1112,11 +1125,32 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
 		goto out_unlock;
 	}

Quoted 14 lines

net/l2tp/l2tp_core.c int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len · line 1139

Quoted 12 lines

net/l2tp/l2tp_core.c +12 -0

+	/* 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 ;)

Quoted 10 lines

net/l2tp/l2tp_core.c int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len · line 1145
Quoted 6 lines
net/l2tp/l2tp_core.c +6 -0
+		 * 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...

Quoted 13 lines

net/l2tp/l2tp_core.c +1 -1

@@ -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;
 		}

Quoted 8 lines

net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1491
Quoted 6 lines
net/l2tp/l2tp_core.c +6 -0
+
+		/* 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.

Quoted 20 lines

net/l2tp/l2tp_core.c +2 ~~-11~~

@@ -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;

Quoted 8 lines

net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1560
Quoted 5 lines
net/l2tp/l2tp_core.c +3 -1
-		}
+		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

Guillaume Nault

2018-03-09 17:47 UTC #5 lore raw

Quoted 29 lines · Paolo Abeni

Hi,

Quoted 17 lines · Guillaume Nault

Quoted 17 lines

net/l2tp/l2tp_core.c +1 -1

index 83421c6f0bef..9726e3f37745 100644
@@ -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;
 		}

Quoted 8 lines

net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1491
Quoted 6 lines
net/l2tp/l2tp_core.c +6 -0
+
+		/* 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.

Quoted 2 lines

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.

Quoted 28 lines

Quoted 19 lines

net/l2tp/l2tp_core.c +2 ~~-11~~

@@ -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;

Quoted 8 lines

net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1560
Quoted 5 lines
net/l2tp/l2tp_core.c +3 -1
-		}
+		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?

Quoted 7 lines

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.

Paolo Abeni

author maintainer

2018-03-09 17:58 UTC #6 lore raw

Quoted 30 lines · Guillaume Nault

Quoted 29 lines · Paolo Abeni

Hi,

Quoted 25 lines · Guillaume Nault

Quoted 14 lines

net/l2tp/l2tp_core.c +1 -1

index 83421c6f0bef..9726e3f37745 100644
@@ -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);

net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1491

Quoted 9 lines

net/l2tp/l2tp_core.c +6 -0

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

Quoted 7 lines

Quoted 2 lines
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

Quoted 31 lines

Quoted 31 lines
Quoted 19 lines
Quoted 19 lines
net/l2tp/l2tp_core.c +2 ~~-11~~
@@ -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;
Quoted 8 lines
net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1560
Quoted 5 lines
net/l2tp/l2tp_core.c +3 -1
-		}
+		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.

Quoted 7 lines

Quoted 4 lines
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

Guillaume Nault

2018-03-09 18:26 UTC #7 lore raw

Quoted 77 lines · Paolo Abeni

Quoted 30 lines · Guillaume Nault
Quoted 29 lines · Paolo Abeni
Hi,
Quoted 25 lines · Guillaume Nault
Quoted 14 lines
net/l2tp/l2tp_core.c +1 -1
index 83421c6f0bef..9726e3f37745 100644
@@ -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);
net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1491
Quoted 9 lines
net/l2tp/l2tp_core.c +6 -0
@@ -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.
Quoted 7 lines
Quoted 2 lines
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
Quoted 36 lines
Quoted 35 lines
Quoted 19 lines
Quoted 19 lines
net/l2tp/l2tp_core.c +2 ~~-11~~
@@ -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;
Quoted 8 lines
net/l2tp/l2tp_core.c int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 · line 1560
Quoted 5 lines
net/l2tp/l2tp_core.c +3 -1
-		}
+		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.

Quoted 8 lines

Quoted 2 lines
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!).

Quoted 4 lines

please have a look at:
https://groups.google.com/forum/#!topic/syzkaller-bugs/N53POqzMUa0

I will, thanks for the link.

Paolo Abeni

author maintainer

2018-03-12 08:53 UTC #8 lore raw

Quoted 9 lines · Guillaume Nault

Quoted 5 lines · Paolo Abeni
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()

Quoted 4 lines

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

Guillaume Nault

2018-03-12 17:11 UTC #9 lore raw

Quoted 14 lines · Paolo Abeni

Quoted 9 lines · Guillaume Nault
Quoted 5 lines · Paolo Abeni
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()

Quoted 8 lines

Quoted 4 lines
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!