From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:51026 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750848AbeCII06 (ORCPT ); Fri, 9 Mar 2018 03:26:58 -0500 Message-ID: <1520584015.2802.1.camel@redhat.com> Subject: Re: [PATCH net 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses From: Paolo Abeni To: Guillaume Nault Cc: netdev@vger.kernel.org, "David S. Miller" , James Chapman , Wei Wang , David Ahern Date: Fri, 09 Mar 2018 09:26:55 +0100 In-Reply-To: <20180308185902.GC1351@alphalink.fr> References: <78591aba68ae107cacbd080d96c5dfb7904be27b.1520510061.git.pabeni@redhat.com> <20180308185902.GC1351@alphalink.fr> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: 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