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

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

On Fri, Mar 09, 2018 at 06:58:00PM +0100, Paolo Abeni wrote:
> On Fri, 2018-03-09 at 18:47 +0100, Guillaume Nault wrote:
> > On Fri, Mar 09, 2018 at 06:04:03PM +0100, Paolo Abeni wrote:
> > > Hi,
> > > 
> > > On Fri, 2018-03-09 at 17:43 +0100, Guillaume Nault wrote:
> > > > > 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
> > > > > @@ -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.
> > > 
> > > 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
> 
> > > > > @@ -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?
> > > 
> > > 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.
> 
> 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!).

> please have a look at:
> 
> https://groups.google.com/forum/#!topic/syzkaller-bugs/N53POqzMUa0
> 
I will, thanks for the link.