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

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

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