netdev - Re: [RFC PATCH] crypto: chtls - 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>] [day] [month] [year] [list]

Date:   Thu, 28 Mar 2019 13:39:44 +0800
From:   Herbert Xu <herbert@...dor.apana.org.au>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     linux-crypto@...r.kernel.org, atul.gupta@...lsio.com,
        pabeni@...hat.com, eric.dumazet@...il.com,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC PATCH] crypto: chtls - handle inet_csk_reqsk_queue_add()
 failures

Guillaume Nault <gnault@...hat.com> wrote:
> While working on 9d3e1368bb45 ("tcp: handle inet_csk_reqsk_queue_add()
> failures"), I missed this usage of inet_csk_reqsk_queue_add().
> 
> inet_csk_reqsk_queue_add() can fail. In that case, the operation is
> aborted and inet_csk_destroy_sock() is called automatically, but we
> need to free the request socket manually.
> 
> RFC:
> Sending as RFC because I'm unsure about how to handle 'child' in that
> case. I don't call add_to_reap_list(child) here, because
> inet_csk_destroy_sock() has already run. Therefore, most of the
> operations performed by add_to_reap_list()/process_reap_list() would be
> redundant (but harmless). Except for two things:
> 
>  * process_reap_list() calls inet_csk_destroy_sock() if ->sk_state is
>    TCP_CLOSE. We have to avoid that. Socket's state should be
>    TCP_ESTABLISHED at the time of the call, but process_reap_list() is
>    run asynchronously and I couldn't find anything that would
>    prevent the rest of the system from accessing the socket before
>    process_reap_list() picks it up (the socket is still accessible
>    from cdev->tids). So I fear that calling add_to_reap_list() in the
>    error path would let process_reap_list() call
>    inet_csk_reqsk_queue_add() a second time if ->sk_state was
>    concurrently set to TCP_CLOSE. That looks like a good reason not to
>    call add_to_reap_list().
> 
>  * The only operation that process_reap_list() does and isn't already
>    done by inet_csk_destroy_sock() is chtls_abort_conn(). This
>    function has no effect if its sk_buff allocation fails. So it might
>    be a no-op. That looks like another reason for not calling
>    add_to_reap_list()...
>    But what chtls_abort_conn() does when sk_buff allocation succeeds
>    looks actually important. IIUC, it sends a message to trigger the
>    removal of the socket (how would the socket be removed from ->tids
>    otherwise?). That would make chtls_abort_conn() quite important.
>    But then how do we avoid losing track of those defunct sockets when
>    sk_buff allocation fails?
> 
> Compile-tested only.
> 
> Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition")
> Reported-by: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Guillaume Nault <gnault@...hat.com>
> ---
> drivers/crypto/chelsio/chtls/chtls_cm.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_cm.c b/drivers/crypto/chelsio/chtls/chtls_cm.c
> index 59b75299fcbc..4bf1e3f65400 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_cm.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_cm.c
> @@ -1384,11 +1384,16 @@ static void add_pass_open_to_parent(struct sock *child, struct sock *lsk,
> 	if (sk_acceptq_is_full(lsk)) {
> 		chtls_reqsk_free(oreq);
> 		add_to_reap_list(child);
> -	} else {
> -		refcount_set(&oreq->rsk_refcnt, 1);
> -		inet_csk_reqsk_queue_add(lsk, oreq, child);
> -		lsk->sk_data_ready(lsk);
> +		return;
> +	}
> +
> +	refcount_set(&oreq->rsk_refcnt, 1);
> +	if (!inet_csk_reqsk_queue_add(lsk, oreq, child)) {
> +		chtls_reqsk_free(oreq);
> +		/* child already dropped by inet_csk_destroy_sock() */
> +		return;
> 	}
> +	lsk->sk_data_ready(lsk);
> }
> 
> static void bl_add_pass_open_to_parent(struct sock *lsk, struct sk_buff *skb)

Dave, Eric, could you guys take a look at this patch please?

Thanks!
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt