Hi,
On Thu, 2018-03-08 at 19:59 +0100, Guillaume Nault wrote:
> On Thu, Mar 08, 2018 at 03:37:27PM +0100, Paolo Abeni wrote:
> > When creating a new socket, l2tp_tunnel_create() ensures that
> > such socket is connected, but when using a socket provided by
> > the user space, no check is done on the socket state.
> >
> > This may foul the later check for ipv6 sockets that are
> > ipv4-mapped, e.g. in case of unconnected ipv6 socket bound to
> > ipv4 address.
> >
> > Moreover the connection status and/or peer of a user-space
> > controlled socket may change at runtime.
> >
> > This change addresses the issues:
> > * explicitly checking for TCP_ESTABLISHED for user space provided sockets
> > * dropping the v4mapped flag usage - it can become outdated - and
> > explicitly invoking ipv6_addr_v4mapped() instead
> > * refreshing the inet_sk copy of ipv4-mapped ipv6 address at xmit time.
> >
> > The issue is apparently there since ancient times.
> >
>
> Thanks, I started working on this report, but you've been faster :)
>
> Here's a quick review, I haven't had time to test yet.
>
> > @@ -1130,15 +1150,13 @@ int l2tp_xmit_skb(struct l2tp_session *session,
> > struct sk_buff *skb, int hdr_len
> > uh->len = htons(udp_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_v4mapped(sk))
>
> So we're going to call udp6_set_csum() only for v4mapped sockets and
> udp_set_csum() for all others (including plain AF_INET6 sockets)?
> I that really the intent, or have I missed something?
It's a bug, thanks for spotting it!
> > udp6_set_csum(udp_get_no_check6_tx(sk),
> > skb, &inet6_sk(sk)->saddr,
> > &sk->sk_v6_daddr, udp_len);
> > else
> > -#endif
> > - udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
> > - inet->inet_daddr, udp_len);
> > + udp_set_csum(sk->sk_no_check_tx, skb, inet->inet_saddr,
> > + inet->inet_daddr, udp_len);
> > break;
> >
> > case L2TP_ENCAPTYPE_IP:
> > @@ -1449,6 +1467,13 @@ 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_err("tunl %u: sock fd=%d is unconnected\n",
> > + tunnel_id, fd);
>
> Following Eric's comment in 17cfe79a65f9 ("l2tp: do not accept arbitrary
> sockets"),
> pr_err() should be avoided here.
>
> Also, 'err' needs to be set before jumping.
Right, thanks.
> > + goto err;
> > + }
> > }
> >
> > diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
> > index a1aa9550f04e..c042aaeb074b 100644
> > --- a/net/l2tp/l2tp_core.h
> > +++ b/net/l2tp/l2tp_core.h
> > @@ -214,6 +211,16 @@ static inline void *l2tp_session_priv(struct
> > l2tp_session *session)
> > return &session->priv[0];
> > }
> >
> > +static bool l2tp_sk_is_v4mapped(struct sock *sk)
>
> The 'inline' keyword is missing, making gcc complain about unused
> function. But since this function is only used in l2tp_core.c, maybe we
> could just move it there.
Agreed.
I will cover the above in v2.
Thank you for the review.
Paolo
