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?
> 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.
> + 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.
