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
> > @@ -1112,11 +1125,32 @@ int l2tp_xmit_skb(struct l2tp_session *session,
> > struct sk_buff *skb, int hdr_len
> > goto out_unlock;
> > }
> >
> > + /* The user-space may change the connection status for the user-space
> > + * provided socket at run time: we must check it under the socket lock
> > + */
> > + inet = inet_sk(sk);
> > + if (tunnel->fd >= 0) {
> > + if (sk->sk_state != TCP_ESTABLISHED) {
> > + ret = NET_XMIT_DROP;
> > + goto out_unlock;
> > + }
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + /* If the uses space changes the ipv4-mapped ipv6 address,
>
> I guess you meant 'user-space'.
yep ;)
> > + * the kernel copy of the ipv4 address is not updated.
> > + * Refresh it only if needed, to avoid dirtying the socket
> > + * on each packet.
> > + */
> > + if (l2tp_sk_is_v4mapped(sk) &&
> > + inet->inet_daddr != sk->sk_v6_daddr.s6_addr32[3])
>
> I can't see how to trigger this condition. Re-connecting the socket
> doesn't do, as __ip6_datagram_connect() already updates both
> ->inet_daddr and ->sk_v6_daddr.
Uh, apparently I trusted too much the original code below...
> > @@ -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.
>
> > @@ -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.
I will send a v3.
Thanks,
Paolo
