netdev - Re: [PATCH net] tcp: handle inet_csk_reqsk_queue

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: <ec516ec8-5de8-beeb-eb79-c7c7c6f8ebbe@gmail.com>
Date:   Fri, 8 Mar 2019 13:33:02 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Guillaume Nault <gnault@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] tcp: handle inet_csk_reqsk_queue_add() failures



On 03/08/2019 01:09 PM, Guillaume Nault wrote:
> Commit 7716682cc58e ("tcp/dccp: fix another race at listener
> dismantle") let inet_csk_reqsk_queue_add() fail, and adjusted
> {tcp,dccp}_check_req() accordingly. However, TFO and syncookies
> weren't modified, thus leaking allocated resources on error.
> 
> Contrary to tcp_check_req(), in both syncookies and TFO cases,
> we need to drop the request socket. Also, since the child socket is
> created with inet_csk_clone_lock(), we have to unlock it and drop an
> extra reference (->sk_refcount is initially set to 2 and
> inet_csk_reqsk_queue_add() drops only one ref).
> 
> For TFO, we also need to revert the work done by tcp_try_fastopen()
> (with reqsk_fastopen_remove()).
> 
> Fixes: 7716682cc58e ("tcp/dccp: fix another race at listener dismantle")
> Signed-off-by: Guillaume Nault <gnault@...hat.com>
> ---
> 
> Note for stable backports: this patch relies on da8ab57863ed
> ("tcp/dccp: remove reqsk_put() from inet_child_forget()"), to prevent
> inet_child_forget() from dropping a reference from the request socket.
> 
> Therefore, for trees older than 4.14, commit da8ab57863ed has to be
> backported before this patch.
> 

Thanks for working on this issue (it was on my radar as well)

> 
>  net/ipv4/syncookies.c | 7 ++++++-
>  net/ipv4/tcp_input.c  | 8 +++++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index 606f868d9f3f..e531344611a0 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -216,7 +216,12 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
>  		refcount_set(&req->rsk_refcnt, 1);
>  		tcp_sk(child)->tsoffset = tsoff;
>  		sock_rps_save_rxhash(child, skb);
> -		inet_csk_reqsk_queue_add(sk, req, child);
> +		if (!inet_csk_reqsk_queue_add(sk, req, child)) {
> +			bh_unlock_sock(child);
> +			sock_put(child);
> +			child = NULL;
> +			reqsk_put(req);

Since we use reqsk_free(req) in the same function, we can use reqsk_free(req)
here as well ?

I suggest the following maybe :

diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 606f868d9f3fde1c3140aa7eecde87d2ec32b5f2..8b28fb66a8fcefba27a2f5e371e9469d4d7e3650 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -216,11 +216,14 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
                refcount_set(&req->rsk_refcnt, 1);
                tcp_sk(child)->tsoffset = tsoff;
                sock_rps_save_rxhash(child, skb);
-               inet_csk_reqsk_queue_add(sk, req, child);
-       } else {
-               reqsk_free(req);
+               if (likely(inet_csk_reqsk_queue_add(sk, req, child)))
+                       return child;
+               bh_unlock_sock(child);
+               sock_put(child);
        }
-       return child;
+
+       reqsk_free(req);
+       return NULL;
 }
 EXPORT_SYMBOL(tcp_get_cookie_sock);
 


> +		}
>  	} else {
>  		reqsk_free(req);
>  	}
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 4eb0c8ca3c60..5def3c48870e 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6498,7 +6498,13 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>  		af_ops->send_synack(fastopen_sk, dst, &fl, req,
>  				    &foc, TCP_SYNACK_FASTOPEN);
>  		/* Add the child socket directly into the accept queue */
> -		inet_csk_reqsk_queue_add(sk, req, fastopen_sk);
> +		if (!inet_csk_reqsk_queue_add(sk, req, fastopen_sk)) {
> +			reqsk_fastopen_remove(fastopen_sk, req, false);
> +			bh_unlock_sock(fastopen_sk);
> +			sock_put(fastopen_sk);



> +			reqsk_put(req);
> +			goto drop;
	These two lines can be replaced by :

	goto  drop_and_free;  

> +		}
>  		sk->sk_data_ready(sk);
>  		bh_unlock_sock(fastopen_sk);
>  		sock_put(fastopen_sk);
>