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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180309174750.GE1351@alphalink.fr>
Date:   Fri, 9 Mar 2018 18:47:50 +0100
From:   Guillaume Nault <g.nault@...halink.fr>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        James Chapman <jchapman@...alix.com>,
        Wei Wang <weiwan@...gle.com>, David Ahern <dsahern@...il.com>
Subject: Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6
 addresses

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.

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