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

[Paolo Abeni <pabeni@redhat.com>]

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