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

[Guillaume Nault <g.nault@alphalink.fr>]

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.