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.
