netdev - Re: [PATCH] tcp: fix syncookie regression

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120312203034.GB24431@hostway.ca>
Date:	Mon, 12 Mar 2012 13:30:34 -0700
From:	Simon Kirby <sim@...nation.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] tcp: fix syncookie regression

On Sat, Mar 10, 2012 at 11:20:21AM -0800, Eric Dumazet wrote:

> commit ea4fc0d619 (ipv4: Don't use rt->rt_{src,dst} in ip_queue_xmit())
> added a serious regression on synflood handling.
> 
> Simon Kirby discovered a successful connection was delayed by 20 seconds
> before being responsive.
> 
> In my tests, I discovered that xmit frames were lost, and needed ~4
> retransmits and a socket dst rebuild before being really sent.
> 
> In case of syncookie initiated connection, we use a different path to
> initialize the socket dst, and inet->cork.fl.u.ip4 is left cleared.
> 
> As ip_queue_xmit() now depends on inet flow being setup, fix this by
> copying the temp flowi4 we use in cookie_v4_check().
> 
> Reported-by: Simon Kirby <sim@...nation.com>
> Bisected-by: Simon Kirby <sim@...nation.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
> Tested-by: Eric Dumazet <eric.dumazet@...il.com>
> ---
>  net/ipv4/syncookies.c |   30 ++++++++++++++++--------------
>  net/ipv4/tcp_ipv4.c   |   10 +++++++---
>  2 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index 51fdbb4..eab2a7f 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -278,6 +278,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
>  	struct rtable *rt;
>  	__u8 rcv_wscale;
>  	bool ecn_ok = false;
> +	struct flowi4 fl4;
>  
>  	if (!sysctl_tcp_syncookies || !th->ack || th->rst)
>  		goto out;
> @@ -346,20 +347,16 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
>  	 * hasn't changed since we received the original syn, but I see
>  	 * no easy way to do this.
>  	 */
> -	{
> -		struct flowi4 fl4;
> -
> -		flowi4_init_output(&fl4, 0, sk->sk_mark, RT_CONN_FLAGS(sk),
> -				   RT_SCOPE_UNIVERSE, IPPROTO_TCP,
> -				   inet_sk_flowi_flags(sk),
> -				   (opt && opt->srr) ? opt->faddr : ireq->rmt_addr,
> -				   ireq->loc_addr, th->source, th->dest);
> -		security_req_classify_flow(req, flowi4_to_flowi(&fl4));
> -		rt = ip_route_output_key(sock_net(sk), &fl4);
> -		if (IS_ERR(rt)) {
> -			reqsk_free(req);
> -			goto out;
> -		}
> +	flowi4_init_output(&fl4, 0, sk->sk_mark, RT_CONN_FLAGS(sk),
> +			   RT_SCOPE_UNIVERSE, IPPROTO_TCP,
> +			   inet_sk_flowi_flags(sk),
> +			   (opt && opt->srr) ? opt->faddr : ireq->rmt_addr,
> +			   ireq->loc_addr, th->source, th->dest);
> +	security_req_classify_flow(req, flowi4_to_flowi(&fl4));
> +	rt = ip_route_output_key(sock_net(sk), &fl4);
> +	if (IS_ERR(rt)) {
> +		reqsk_free(req);
> +		goto out;
>  	}
>  
>  	/* Try to redo what tcp_v4_send_synack did. */
> @@ -373,5 +370,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
>  	ireq->rcv_wscale  = rcv_wscale;
>  
>  	ret = get_cookie_sock(sk, skb, req, &rt->dst);
> +	/* ip_queue_xmit() depends on our flow being setup
> +	 * Normal sockets get it right from inet_csk_route_child_sock()
> +	 */
> +	if (ret)
> +		inet_sk(ret)->cork.fl.u.ip4 = fl4;
>  out:	return ret;
>  }
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 94d683a..fd54c5f 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1466,9 +1466,13 @@ struct sock *tcp_v4_syn_recv_sock(struct sock *sk, struct sk_buff *skb,
>  		inet_csk(newsk)->icsk_ext_hdr_len = inet_opt->opt.optlen;
>  	newinet->inet_id = newtp->write_seq ^ jiffies;
>  
> -	if (!dst && (dst = inet_csk_route_child_sock(sk, newsk, req)) == NULL)
> -		goto put_and_exit;
> -
> +	if (!dst) {
> +		dst = inet_csk_route_child_sock(sk, newsk, req);
> +		if (!dst)
> +			goto put_and_exit;
> +	} else {
> +		/* syncookie case : see end of cookie_v4_check() */
> +	}
>  	sk_setup_caps(newsk, dst);
>  
>  	tcp_mtup_init(newsk);

Tested under real SYN flood on 3.3-rc7 and 3.2.9 -- works! Thanks!

Simon-
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html