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.
